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

[Dan]

On Tue, May 16, 2017 at 11:41:55AM +0200, Martin Kletzander wrote:
> 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
> 
I made changes accordingly in the version 2 of this patch.
> > 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.
> 
I kept the existing code and only added an additional check for "domain"
in v2, i.e.:

    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
        vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0 ||
        vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
        return false;
> > +    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.
> 
I did not fully get it by "getting rid of the lines above [1]." But I
made the change as you suggested to use NULL as the third argument.
> 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.
> 
Yep, I changed it accordingly.
> >     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

I changed it to something like:
if (!(configData = virConnectDomainXMLToNative())) {
        return false;
        goto cleanup;
} else {
       vshPrint();
}

Hopefully I am following the libvirt convention here. Let me know if
there is anything more needs to be changed.

Thank you very much,

Dan