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

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".

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