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

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