On 05/02/2016 09:59 AM, Nitesh Konkar wrote:

virshDomainDetachInterface handles virsh interface detach from the specified live/config domain xml.

Signed-off-by: Nitesh Konkar <nitkon12@linux.vnet.ibm.com> --- tools/virsh-domain.c | 104 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 61 insertions(+), 43 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 0a6caae..d9fde4d 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11199,39 +11199,22 @@ static const vshCmdOptDef opts_detach_interface[] = { };

static bool -cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) +virshDomainDetachInterface(char *doc, + unsigned int flags, + virDomainPtr dom, + vshControl *ctl, + const vshCmd *cmd) {

The split is a bit wrong IMO. I don't think virshDomainDetachInterface should be doing any command line processing. I think the args should be: virshDomainDetachInterface(vshControl *ctl, virDomainPtr dom, unsigned int flags, bool current, const char *type, const char *mac); One other comment below:

- virDomainPtr dom = NULL; xmlDocPtr xml = NULL; xmlXPathObjectPtr obj = NULL; xmlXPathContextPtr ctxt = NULL; xmlNodePtr cur = NULL, matchNode = NULL; - char *detach_xml = NULL; const char *mac = NULL, *type = NULL; - char *doc = NULL; - char buf[64]; - int diff_mac; + char *detach_xml = NULL, buf[64]; + bool current = vshCommandOptBool(cmd, "current"); + int diff_mac, ret; size_t i; - int ret; bool functionReturn = false; - unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; - bool current = vshCommandOptBool(cmd, "current"); - bool config = vshCommandOptBool(cmd, "config"); - bool live = vshCommandOptBool(cmd, "live"); - bool persistent = vshCommandOptBool(cmd, "persistent"); - - VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current); - - VSH_EXCLUSIVE_OPTIONS_VAR(current, live); - VSH_EXCLUSIVE_OPTIONS_VAR(current, config); - - if (config || persistent) - flags |= VIR_DOMAIN_AFFECT_CONFIG; - if (live) - flags |= VIR_DOMAIN_AFFECT_LIVE; - - if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) - return false;

if (vshCommandOptStringReq(ctl, cmd, "type", &type) < 0) goto cleanup; @@ -11239,18 +11222,6 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0) goto cleanup;

- if (persistent && - virDomainIsActive(dom) == 1) - flags |= VIR_DOMAIN_AFFECT_LIVE; - - if (flags & VIR_DOMAIN_AFFECT_CONFIG) - doc = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE); - else - doc = virDomainGetXMLDesc(dom, 0); - - if (!doc) - goto cleanup; - if (!(xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt))) { vshError(ctl, "%s", _("Failed to get interface information")); goto cleanup; @@ -11315,20 +11286,67 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) else ret = virDomainDetachDevice(dom, detach_xml);

- if (ret != 0) { + if (ret == 0) + functionReturn = true; + + cleanup: + VIR_FREE(detach_xml); + xmlFreeDoc(xml); + xmlXPathFreeObject(obj); + xmlXPathFreeContext(ctxt); + return functionReturn; +} + + +static bool +cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + char *doc = NULL; + bool functionReturn = false; + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + bool current = vshCommandOptBool(cmd, "current"); + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool persistent = vshCommandOptBool(cmd, "persistent"); + + VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current); + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config || persistent) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (persistent && + virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) + doc = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE); + else + doc = virDomainGetXMLDesc(dom, 0); + + if (!doc) + goto cleanup; + else + functionReturn = virshDomainDetachInterface(doc, flags, dom, ctl, cmd); + + cleanup: + if (functionReturn == false) { vshError(ctl, "%s", _("Failed to detach interface")); } else { vshPrint(ctl, "%s", _("Interface detached successfully\n")); functionReturn = true; }

- cleanup: VIR_FREE(doc); - VIR_FREE(detach_xml); virDomainFree(dom); - xmlXPathFreeObject(obj); - xmlXPathFreeContext(ctxt); - xmlFreeDoc(xml); return functionReturn;

Drop functionReturn entirely here. set ret=-1 at init time, then return ret == 0; Otherwise it looks good. Thanks, Cole

On 05/02/2016 09:59 AM, Nitesh Konkar wrote:

cmdDetachInterface function checks for live config flags and then passes the live/config domain xml to virshDomainDetachInterface accordingly.

Signed-off-by: Nitesh Konkar <nitkon12@linux.vnet.ibm.com> --- tools/virsh-domain.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index d9fde4d..5cfd6a3 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11302,7 +11302,7 @@ static bool cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; - char *doc = NULL; + char *doc_live = NULL, *doc_config = NULL; bool functionReturn = false; unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; bool current = vshCommandOptBool(cmd, "current"); @@ -11317,8 +11317,6 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)

Hmm, I'm having trouble following the logic here with the flag manipulation. I think there's an error here with --persistent handling at least. I suggest to drop the 'flags' variable entirely. So...

if (config || persistent) flags |= VIR_DOMAIN_AFFECT_CONFIG;

Here I'd change to bool affect_config; ... affect_config = (config || persistent);

- if (live) - flags |= VIR_DOMAIN_AFFECT_LIVE;

if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -11327,15 +11325,23 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) virDomainIsActive(dom) == 1) flags |= VIR_DOMAIN_AFFECT_LIVE;

- if (flags & VIR_DOMAIN_AFFECT_CONFIG) - doc = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE); - else - doc = virDomainGetXMLDesc(dom, 0); + if (flags & VIR_DOMAIN_AFFECT_CONFIG) {

This will just be (affect_config)

+ if (!(doc_config = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE))) + goto cleanup; + if (!(functionReturn = virshDomainDetachInterface(doc_config, flags, dom, ctl, cmd))) + goto cleanup; + }

- if (!doc) - goto cleanup; - else - functionReturn = virshDomainDetachInterface(doc, flags, dom, ctl, cmd); + flags = 0; + + if (live || (!live && !config)) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) {

This will be: bool affect_live; ... affect_live = (live || (persistent && virDomainIsActive(dom) == 1)); if (affect_live || !affect_config) int flags = 0; if (affect_live) flags |= VIR_DOMAIN_AFFECT_LIVE; ... The last bit with the 'int flags' ensures we still use flags=0 and old style virDomainDetachDevice() if the user doesn't specify any command line flags. We want to preserve that behavior for compat with older libvirt - Cole