Re: [PATCH] virsh: add --domain option for domain-to-native

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Mon, May 15, 2017 at 04:29:48AM -0400, Daniel Liu wrote:
> Fix bug 835476[1].

It's enough to mention it below (as you did).

> virsh: add [--domain DOMAIN] option to  domxml-to-native DOMAIN COMMAND

This first line is already in the subject.

> Add support for the following syntax:
> domxml-to-native <format> { [--domain DOMAIN] | [XML] }, i.e., it supports
> either designating domain (domain id, uuid, or name), or path to XML domain
> configuration file.

I would reword this a little bit.  How would you feel about something
along the lines of:

 The option allows someone to run domain-to-native on already existing
 domain without the need of supplying their XML.  It is basically
 wrapper around `virsh dumpxml $dom | virsh domxml-to-native /dev/stdin`.

 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=835476

> E.g.:
> virsh domxml-to-native qemu-argv --domain RHEL7.3   # domain name
> virsh domxml-to-native qemu-argv --domain 10        # domain id
> virsh domxml-to-native qemu-argv dumped_dom.xml     # dumped xml
>
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=835476
> ---
> tools/virsh-domain.c | 54 ++++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 44 insertions(+), 10 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 0d19d0e01..a79fd3ab2 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -9840,9 +9840,13 @@ static const vshCmdOptDef opts_domxmltonative[] = {
>      .flags = VSH_OFLAG_REQ,
>      .help = N_("target config data type format")
>     },
> +    {.name = "domain",
> +     .type = VSH_OT_DATA,
> +     .flags = VSH_OFLAG_REQ_OPT,
> +     .help = N_("domain name, id or uuid")
> +    },
>     {.name = "xml",
>      .type = VSH_OT_DATA,
> -     .flags = VSH_OFLAG_REQ,
>      .help = N_("xml data file to export from")
>     },
>     {.name = NULL}
> @@ -9851,30 +9855,60 @@ static const vshCmdOptDef opts_domxmltonative[] = {
> static bool
> cmdDomXMLToNative(vshControl *ctl, const vshCmd *cmd)
> {
> -    bool ret = true;
> +    bool ret = false;
>     const char *format = NULL;
> -    const char *xmlFile = NULL;
> -    char *configData;
> -    char *xmlData;
> +    const char *domain = NULL;
> +    const char *xml = NULL;
> +    char *xmlData = NULL;
> +    char *configData = NULL;
>     unsigned int flags = 0;
>     virshControlPtr priv = ctl->privData;
> +    virDomainPtr dom = NULL;
>
> -    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
> -        vshCommandOptStringReq(ctl, cmd, "xml", &xmlFile) < 0)

If this was already there, you could keep using it.  But that's not a
big deal for me, just some others might not like it.

> +    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0)
> +        return false;
> +
> +    if (vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
> +        return false;
> +

[1] So here you get the domain name/id/uuid ...

> +    if (vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0)
>         return false;
>
> -    if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0)
> +    VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml);
> +
> +    if (domain)
> +        dom = virshCommandOptDomain(ctl, cmd, &domain);
> +

... and here you get the object.  What do you supply as the third
parameter?  Check what the function does.  There's a leak that you will
fix by getting rid of the lines above [1].  And just supply NULL here.

Also make sure &dom is not NULL here, in case someone supplies
non-existing domain name.

> +    if (!dom && !xml) {
> +        vshError(ctl, _("need either domain (ID, UUID, or name) or domain XML configuration file path"));
>         return false;
> +    }
> +
> +    if (dom) {
> +        xmlData = virDomainGetXMLDesc(dom, flags);
> +        if (xmlData == NULL)
> +            goto cleanup;
> +    }
> +
> +    if (xml) {
> +        if (virFileReadAll(xml, VSH_MAX_XML_FILE, &xmlData) < 0)
> +            goto cleanup;
> +    }

This ^^ would be more readable as:

if (dom) {
  xmlData = blah;
} else if (xml) {
  readFile(asdf, &xmlData);
}

if (!xmlData) {
 vshError(ctl, "%s",
          _("Either <xml> or --domain must be suppied"));


Or something in that regard.

>     configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags);
>     if (configData != NULL) {
>         vshPrint(ctl, "%s", configData);
> -        VIR_FREE(configData);
> +        ret = true;
> +        goto cleanup;
>     } else {
> -        ret = false;
> +        vshError(ctl, _("convert from domain XML to native command failed"));
> +        goto cleanup;
>     }

Since you are changing this...  This does not look like all the other
places in libvirt.  What we do _almost_ exclusively is:

 something = func();
 if (!func) {
     handle error;
     goto somewhere;
 }

 what = should + continue(in.case->of.no(&error);

 ret = success code;
cleanup:
 stuff to clean up;


I would not normally mention it, but since you are changing that part
anyway, it could be cleaned up so that it does what other parts of the
code do.  Or just keep it as it is.  It's small and right before the
cleanup, so it's _kinda_ OK (i.e. we don't have a rule written down in
the coding style for this).

Otherwise it's fine ;)

Martin

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list