On Sat, Apr 25, 2015 at 12:11 AM, Laine Stump <laine@xxxxxxxxx> wrote:
> On 04/24/2015 06:58 AM, Shivaprasad bhat wrote:
>> Thanks for the patches Laine. I agree pretty much with both the patches.
>> also had a chance to try these out.
>>
>> Only scenario I see a trouble is,
>>
>> net-create without bridge name in the xml, followed by net-define
>> using the same xml file.
>> The live and --inactive xmls both show different bridge names, though
>> the active bridge
>> can be reused by the network for "this scenario alone". I think this
>> is a very rare case and not worth
>> to fix it,
>
> Using the original XML from net-create for a subsequent net-define is
> problematic in other ways too - if you don't specify MAC address you'll
> end up with def and newDef having different MAC addresses, and if you
> don't specify uuid (I would daresay almost nobody does) you will be
> unable to define the network since a different uuid will be
> autogenerated in both cases, and libvirt will refuse to redefine a
> network with the same name but different uuid. So I don't think we
> should consider this same/similar complication with bridge name a problem.
>
> The proper way to make a transient network persistent is this:
>
> virsh net-dumpxml $netname --inactive >/tmp/xyz.xml && \
> virsh net-define /tmp/xyz.xml
>
>
>> as net-destroy followed by net-create using the same xml
>> file would anyway reuse the
>> same old bridge name if available.
>>
>> Outside of this patch,
>> the net->newDef->bridge's are not considered in virNetworkBridgeInUseHelper().
>> Do we need to?
>
> Yes, that's a good idea. I'm posting a patch 3/2 to this series.
>
> You seem to approve of the patches and have tested them; will you be
> ACKing them?
>
>
Certainly! ACK the first two patches.
The third patch, I am leaving a comment there.
Thanks,
Shiva
>>
>> Thanks,
>> Shiva
>>
>> On Fri, Apr 24, 2015 at 12:33 AM, Laine Stump <laine@xxxxxxxxx> wrote:
>>> Since some people use the same naming convention as libvirt for bridge
>>> devices they create outside the context of libvirt, it is much nicer
>>> if we check for those devices when looking for a bridge device name to
>>> auto-assign to a new network.
>>> ---
>>> src/network/bridge_driver.c | 25 +++++++++++++++++--------
>>> 1 file changed, 17 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>>> index abacae3..3b879cd 100644
>>> --- a/src/network/bridge_driver.c
>>> +++ b/src/network/bridge_driver.c
>>> @@ -2037,11 +2037,13 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
>>>
>>> /* Create and configure the bridge device */
>>> if (!network->def->bridge) {
>>> - /* this can only happen if the config files were edited
>>> - * directly. Otherwise networkValidate() (called after parsing
>>> - * the XML from networkCreateXML() and networkDefine())
>>> - * guarantees we will have a valid bridge name before this
>>> - * point.
>>> + /* bridge name can only be empty if the config files were
>>> + * edited directly. Otherwise networkValidate() (called after
>>> + * parsing the XML from networkCreateXML() and
>>> + * networkDefine()) guarantees we will have a valid bridge
>>> + * name before this point. Since hand editing of the config
>>> + * files is explicitly prohibited we can, with clear
>>> + * conscience, log an error and fail at this point.
>>> */
>>> virReportError(VIR_ERR_INTERNAL_ERROR,
>>> _("network '%s' has no bridge name defined"),
>>> @@ -2762,8 +2764,9 @@ static int networkIsPersistent(virNetworkPtr net)
>>>
>>> /*
>>> * networkFindUnusedBridgeName() - try to find a bridge name that is
>>> - * unused by the currently configured libvirt networks, and set this
>>> - * network's name to that new name.
>>> + * unused by the currently configured libvirt networks, as well as by
>>> + * the host system itself (possibly created by someone/something other
>>> + * than libvirt). Set this network's name to that new name.
>>> */
>>> static int
>>> networkFindUnusedBridgeName(virNetworkObjListPtr nets,
>>> @@ -2777,7 +2780,13 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets,
>>> do {
>>> if (virAsprintf(&newname, templ, id) < 0)
>>> goto cleanup;
>>> - if (!virNetworkBridgeInUse(nets, newname, def->name)) {
>>> + /* check if this name is used in another libvirt network or
>>> + * there is an existing device with that name. ignore errors
>>> + * from virNetDevExists(), just in case it isn't implemented
>>> + * on this platform (probably impossible).
>>> + */
>>> + if (!(virNetworkBridgeInUse(nets, newname, def->name) ||
>>> + virNetDevExists(newname) == 1)) {
>>> VIR_FREE(def->bridge); /*could contain template */
>>> def->bridge = newname;
>>> ret = 0;
>>> --
>>> 2.1.0
>>>
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list