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

[Lin Ma]

>>>> Laine Stump <laine at laine.org> 2015-2-4 下午 17:12 >>>
>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 
>>> ---
>>>  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).
It seems like that we decided to keep the original net-destroy behaviour, Then
let's keep iface-unbridge's too.

>>
>>> 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.
As Daniel points out, destroy is The net-destroy already shows to user that 
it's a forceful operation, So we don't need a --force option, then iface-unbridge either.



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150205/565b3a7f/attachment-0001.htm>