[libvirt] [PATCH] RFE: virsh: add domxml-to-native <fmt> [--domain DOMAIN] option

[Dan]

On Mon, Apr 24, 2017 at 09:17:12AM +0200, Peter Krempa wrote:
> On Sun, Apr 23, 2017 at 20:54:47 -0400, Dan wrote:
> 
> Please use your full name for patch submissions.
> 
I just did a new send-email patch submission to the list. Hopefully it
corrected my previous mistakes.
> > Bug 835476 RFE: virsh: add domxml-to-native --domain option (for existing
> > VM) [1]
> > 
> > virsh DOMAIN COMMAND domxml-to-native did not support domain (id|uuid|name)
> > as input for generating hypervisor agent native command, instead only
> > supported
> > XML input from STDIN. Here in this patch, it supports 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; NOTE that it deprecated existing STDIN input passing XML
> > functionality. It was tested on the test mock driver and QEMU.
> > 
> > 
> > 
> > 
> > [1]. https://bugzilla.redhat.com/show_bug.cgi?id=835476
> > 
> > 
> > 
> > 
> > Dan
> 
> This whole block would get added to the commit message. You should not
> put anything into it which you don't want there. You want to describe
> the change briefly, but that's it.
> 
> 
> > 
> > ---
> >  tools/virsh-domain.c | 68
> > ++++++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 55 insertions(+), 13 deletions(-)
> 
> This patch is corrupted. Usually it's the best to use git-send-email to
> post patches. Alternatively you can attach the file itself. Pasting the
> patch into your mail client will corrupt it most probably.
> 
> Please re-send a non-corrupted version with your proper name as the
> author.
>
I configured my git send-email. Please let me know if there is still
format issues.

> Also if you didn't make so, make sure you run make check and make
> syntax-check before posting patches.
> 
This time I ran both of the checks, except that "make check" skipped
test.
> > 
> > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> > index db8accfe4..9ac855b19 100644
> > --- a/tools/virsh-domain.c
> > +++ b/tools/virsh-domain.c
> > @@ -60,6 +60,7 @@
> >  #include "virsh-nodedev.h"
> >  #include "viruri.h"
> > 
> > +
> 
> Spurious whitespace addition.
> 
I deleted it. Thank you very much for pointing it out.
> >  /* Gnulib doesn't guarantee SA_SIGINFO support.  */
> >  #ifndef SA_SIGINFO
> >  # define SA_SIGINFO 0
> > @@ -9811,9 +9812,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}
> > @@ -9822,31 +9827,68 @@ static const vshCmdOptDef opts_domxmltonative[] = {
> >  static bool
> >  cmdDomXMLToNative(vshControl *ctl, const vshCmd *cmd)
> >  {
> > -    bool ret = true;
> >      const char *format = NULL;
> >      const char *xmlFile = NULL;
> > -    char *configData;
> > -    char *xmlData;
> > +    const char *domain = NULL;
> > +    char *configData = NULL;
> > +    char *xmlData = NULL;
> >      unsigned int flags = 0;
> > +    unsigned int domflags = 0;
> >      virshControlPtr priv = ctl->privData;
> > +    virDomainPtr dom = NULL;
> > 
> > -    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
> > -        vshCommandOptStringReq(ctl, cmd, "xml", &xmlFile) < 0)
> > +    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0)
> >          return false;
> > +
> > +    if (vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
> > + return false;
> > 
> > -    if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0)
> > -        return false;
> > +    if (vshCommandOptStringReq(ctl, cmd, "xml", &xmlFile) < 0)
> > + return false;
> > +
> > +    VSH_EXCLUSIVE_OPTIONS_VAR(domain, xmlFile);
> > +
> > +    if (domain) {
> > + domflags = VIRSH_BYID | VIRSH_BYUUID | VIRSH_BYNAME;
> > +        dom = virshLookupDomainBy(ctl, domain, domflags);
> 
> You can use virshCommandOptDomain instead of this. And the string
> lookup.
> 
Oh yeah, I used it this time. I would not know its existence without you
telling me. So many options to achieve one thing. But this seems to be
the most elegant way.

Thanks a lot,

Dan
> > +    }
> > +
> > +    if (!dom && !xmlFile) {