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

It would be nice to include the changes into documentation, wouldn't it? tools/virsh.pod has an entry for example.

2017-05-29 16:40 GMT-03:00 Dan <srwx4096(a)gmail.com&gt;: > On Mon, May 29, 2017 at 08:53:18PM +0200, Martin Kletzander wrote: >> On Mon, May 29, 2017 at 02:25:03PM -0400, Dan wrote: >> > 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 >> >> I meant the three lines before the [1] (I used that number in square >> brackets as a link. That's why I said "Check what the function does". > > So I deleted the line checking "domain" leaving origin conditional like > as it was: > > if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 || > vshCommandOptStringReq(ctl, cmd, "xml", &xmlFile) < 0) > > However, this change would reulst in error when supplying "domain" as the > string of domain needs to be accquired, which was achieved by calling > vshCommandOptStringReq(ctl, cmd, "domain", &domain). > > So far, the exception handeling and normal return are the following: > > > $ virsh -c qemu:///system list --all > Id Name State > ---------------------------------------------------- > 2 ubuntu1404 running > - RHEL7.3 shut off > > $ virsh -c qemu:///system domxml-to-native > error: command 'domxml-to-native' requires <format> option > > $ virsh -c qemu:///system domxml-to-native qemu-argv > error: need either domain (ID, UUID, or name) or domain XML configuration file path > > $ virsh -c qemu:///system domxml-to-native qemu-argv 2 > error: Failed to open file '2': No such file or directory > > $ virsh -c qemu:///system domxml-to-native qemu-argv --domain 2 => OK > > $ virsh -c qemu:///system domxml-to-native qemu-argv dl_dumpxml170422.xml => OK > > $ virsh -c qemu:///system domxml-to-native qemu-argv dl_dumpxml170529.xml # Wrong XML > error: convert from domain XML to native command failed > error: (domain_definition):111: expected '>' > </domain> > -------^ > > $ virsh -c qemu:///system domxml-to-native qemu-argv --xml dl_dumpxml170422.xml => OK > > $ virsh -c qemu:///system domxml-to-native qemu-argv --xml dl_dumpxml170422.xml --domain > error: expected syntax: --domain <string> > > $ virsh -c qemu:///system domxml-to-native qemu-argv --xml dl_dumpxml170422.xml --domain 3 > error: Options --domain and --xml are mutually exclusive > > $ virsh -c qemu:///system domxml-to-native qemu-argv --domain 3 > error: failed to get domain '3' > error: Domain not found: no domain with matching name '3' > > Best, > > Dan > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list