[libvirt] [PATCH 1/2] virsh: Introduce virshDomainDetachInterface function
Cole Robinson
crobinso at redhat.com
Mon May 2 23:31:01 UTC 2016
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 at 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
More information about the libvir-list
mailing list