[libvirt] [PATCH 2/2] network: check for bridge name conflict with existing devices

[Shivaprasad bhat]

On Sat, Apr 25, 2015 at 12:11 AM, Laine Stump <laine at laine.org> 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 at laine.org> 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list