[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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?


>
> Thanks,
> Shiva
>
> On Fri, Apr 24, 2015 at 12:33 AM, Laine Stump <laine 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
>>


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]