[libvirt] [PATCH 3/3] virsh: prevent removing a in-used bridge for iface-unbridge

[Laine Stump]

On 02/03/2015 11:39 AM, Michal Privoznik wrote:
> On 02.02.2015 15:08, Lin Ma wrote:
>> By checking transient interfaces, It obtains the live information of
>> attached interfaces to see if the bridge is in use.
>>
>> Signed-off-by: Lin Ma <lma at suse.com>
>> ---
>>  tools/virsh-interface.c | 25 ++++++++++++++++++++++---
>>  1 file changed, 22 insertions(+), 3 deletions(-)
> Technically, this is a v2 to a previous patch (I mildly recall seeing
> something like this in the past).

It looks to be the same patch, just with reference to a private bug
report removed, and preceded with the check for net-destroy (since I had
said in my response to the original patch that the behavior of
iface-unbridge was made to be similar to net-destroy, and that my
opinion was that either neither should be changed, or both).

>
>> diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c
>> index 5f848b6..ff40be0 100644
>> --- a/tools/virsh-interface.c
>> +++ b/tools/virsh-interface.c
>> @@ -1040,11 +1040,11 @@ cmdInterfaceUnbridge(vshControl *ctl, const vshCmd *cmd)
>>      const char *br_name;
>>      char *if_type = NULL, *if_name = NULL;
>>      bool nostart = false;
>> -    char *br_xml = NULL;
>> +    char *br_xml = NULL, *br_xml_transient_if = NULL;
>>      xmlChar *if_xml = NULL;
>>      int if_xml_size;
>> -    xmlDocPtr xml_doc = NULL;
>> -    xmlXPathContextPtr ctxt = NULL;
>> +    xmlDocPtr xml_doc = NULL, xml_doc_transient_if = NULL;
>> +    xmlXPathContextPtr ctxt = NULL, ctxt_transient_if = NULL;
>>      xmlNodePtr top_node, if_node, cur;
>>  
>>      /* Get a handle to the original device */
>> @@ -1103,6 +1103,22 @@ cmdInterfaceUnbridge(vshControl *ctl, const vshCmd *cmd)
>>          goto cleanup;
>>      }
>>  
>> +    /* verify whether there is any transient interface attached to bridge. */
>> +    if (!(br_xml_transient_if = virInterfaceGetXMLDesc(br_handle, 0)))
>> +        goto cleanup;
>> +
>> +    if (!(xml_doc_transient_if = virXMLParseStringCtxt(br_xml_transient_if,
>> +                                                       _("(bridge interface definition)"),
>> +                                                       &ctxt_transient_if))) {
>> +        vshError(ctl, _("Failed to parse configuration of %s"), br_name);
>> +        goto cleanup;
>> +    }
>> +
>> +    if (virXPathNode("./bridge/interface[2]", ctxt_transient_if) != NULL) {
>> +        vshError(ctl, "%s", _("The bridge is in use by transient interfaces"));
>> +        goto cleanup;
>> +    }
>> +
>>      /* Change the type and name of the outer/master interface to
>>       * the type/name of the attached slave interface.
>>       */
>> @@ -1198,10 +1214,13 @@ cmdInterfaceUnbridge(vshControl *ctl, const vshCmd *cmd)
>>         virInterfaceFree(br_handle);
>>      VIR_FREE(if_xml);
>>      VIR_FREE(br_xml);
>> +    VIR_FREE(br_xml_transient_if);
>>      VIR_FREE(if_type);
>>      VIR_FREE(if_name);
>>      xmlXPathFreeContext(ctxt);
>> +    xmlXPathFreeContext(ctxt_transient_if);
>>      xmlFreeDoc(xml_doc);
>> +    xmlFreeDoc(xml_doc_transient_if);
>>      return ret;
>>  }
>>  
>>
> ACK. I'll merge this tomorrow (unless somebody beats me).

Please don't push it as is. I think the behavior of iface-unbridge
should match whatever is done to net-destroy (if anything). If the
change is made, it should be made to both at the same time, and this one
should also have a --force option to allow overriding the extra check,
as patch 2/3 does for net-destroy.

(BTW, I'm still curious about Suse's use of netcf, as there have been no
updates to the Suse driver in netcf since the initial port was imported
several years ago. Are there downstream changes that can be sent upstream?)