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.