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 | virsh domxml-to-native /dev/stdin'. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=835476 Signed-off-by: Daniel Liu <srwx4096(a)gmail.com&gt; --- tools/virsh-domain.c | 51 ++++++++++++++++++++++++++++++++++++++------------- tools/virsh.pod | 7 ++++--- 2 files changed, 42 insertions(+), 16 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ccb514ef9..3496359c5 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9848,9 +9848,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} @@ -9859,30 +9863,51 @@ 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 *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) + vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0) return false; - if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0) - return false; + VSH_EXCLUSIVE_OPTIONS("domain", "xml"); - configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags); - if (configData != NULL) { - vshPrint(ctl, "%s", configData); - VIR_FREE(configData); + if (vshCommandOptBool(cmd, "domain") && + (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))) + return false; + + if (dom) { + xmlData = virDomainGetXMLDesc(dom, flags); + } else if (xml) { + if (virFileReadAll(xml, VSH_MAX_XML_FILE, &xmlData) < 0) + goto cleanup; } else { - ret = false; + vshError(ctl, "%s", _("need either domain or domain XML")); + goto cleanup; + } + + if (!xmlData) { + vshError(ctl, "%s", _("failed to retrieve XML")); + goto cleanup; } + if (!(configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags))) { + goto cleanup; + } else { + vshPrint(ctl, "%s", configData); + ret = true; + } + + cleanup: + virshDomainFree(dom); VIR_FREE(xmlData); + VIR_FREE(configData); return ret; } diff --git a/tools/virsh.pod b/tools/virsh.pod index aee964689..049c2f3c7 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1433,10 +1433,11 @@ the I<format> argument must be B<qemu-argv>. For Xen hypervisor, the I<format> argument may be B<xen-xm>, B<xen-xl>, or B<xen-sxpr>. For LXC hypervisor, the I<format> argument must be B<lxc-tools>. -=item B<domxml-to-native> I<format> I<xml> +=item B<domxml-to-native> I<format> +{ I<--domain> I<domain-name-or-id-or-uuid> | [I<--xml>] I<xml> }

-Convert the file I<xml> in domain XML format to the native guest -configuration format named by I<format>. For QEMU/KVM hypervisor, +Convert the file I<xml> in domain XML format or existing domain to the +native guest configuration format named by I<format>. For QEMU/KVM hypervisor, the I<format> argument must be B<qemu-argv>. For Xen hypervisor, the I<format> argument may be B<xen-xm>, B<xen-xl>, or B<xen-sxpr>. For LXC hypervisor, the I<format> argument must be B<lxc-tools>.

On Wed, Jun 14, 2017 at 08:55:25AM -0400, John Ferlan wrote: > > > On 06/02/2017 11:04 AM, Daniel Liu wrote: >> 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 | virsh domxml-to-native /dev/stdin'. >> >> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=835476 >> Signed-off-by: Daniel Liu <srwx4096(a)gmail.com&gt; >> --- >> tools/virsh-domain.c | 51 >> ++++++++++++++++++++++++++++++++++++++------------- >> tools/virsh.pod | 7 ++++--- >> 2 files changed, 42 insertions(+), 16 deletions(-) >> >> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c >> index ccb514ef9..3496359c5 100644 >> --- a/tools/virsh-domain.c >> +++ b/tools/virsh-domain.c >> @@ -9848,9 +9848,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} >> @@ -9859,30 +9863,51 @@ 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 *xml = NULL; > > nit: no need to change from xmlFile to xml... The less differences the > better ;-) > There was no change, it is an additional variable, the original one is below. The number of differences would be the same, I believe.

>> + char *xmlData = NULL; >> + char *configData = NULL; > > nit: no need to change order from original, just add ' = NULL' is fine. > >> unsigned int flags = 0; >> virshControlPtr priv = ctl->privData; >> + virDomainPtr dom = NULL; >> >> if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 || >> - vshCommandOptStringReq(ctl, cmd, "xml", &xmlFile) < 0) >> + vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0)

>> return false; >> >> - if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0) >> - return false; >> + VSH_EXCLUSIVE_OPTIONS("domain", "xml"); >> >> - configData = virConnectDomainXMLToNative(priv->conn, format, >> xmlData, flags); >> - if (configData != NULL) { >> - vshPrint(ctl, "%s", configData); >> - VIR_FREE(configData); >> + if (vshCommandOptBool(cmd, "domain") && >> + (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))) >> + return false; >> + >> + if (dom) { >> + xmlData = virDomainGetXMLDesc(dom, flags); >> + } else if (xml) {

>> + if (virFileReadAll(xml, VSH_MAX_XML_FILE, &xmlData) < 0) >> + goto cleanup; >> } else { >> - ret = false; >> + vshError(ctl, "%s", _("need either domain or domain XML")); >> + goto cleanup; >> + } >> + >> + if (!xmlData) { >> + vshError(ctl, "%s", _("failed to retrieve XML")); >> + goto cleanup; >> } >> >> + if (!(configData = virConnectDomainXMLToNative(priv->conn, >> format, xmlData, flags))) { >> + goto cleanup; >> + } else { >> + vshPrint(ctl, "%s", configData); >> + ret = true; >> + } >> + >> + cleanup: >> + virshDomainFree(dom); >> VIR_FREE(xmlData); >> + VIR_FREE(configData); >> return ret; >> } >> >> diff --git a/tools/virsh.pod b/tools/virsh.pod >> index aee964689..049c2f3c7 100644 >> --- a/tools/virsh.pod >> +++ b/tools/virsh.pod >> @@ -1433,10 +1433,11 @@ the I<format> argument must be B<qemu-argv>. >> For Xen hypervisor, the >> I<format> argument may be B<xen-xm>, B<xen-xl>, or B<xen-sxpr>. For >> LXC hypervisor, the I<format> argument must be B<lxc-tools>. >> >> -=item B<domxml-to-native> I<format> I<xml> >> +=item B<domxml-to-native> I<format> >> +{ I<--domain> I<domain-name-or-id-or-uuid> | [I<--xml>] I<xml> } > > This looks really odd, I think this should be > > { [I<xml> | [I<--domain> I<domain-name-or-id-uuid>] } > Actually, the only thing that Dan can change is the order, but the rest should remain. His version shows that you must supply either --domain domain-name-or-id-or-uuid or --xml xml and that in the second case, the --xml is optional. Your suggested solution, however, mean that you must supply either the xml (no mention about the optional --xml) or --domain domain-name-or-id-or-uuid or nothing (in a really weird way), as the latter option is in square brackets. So the only way this could look differently is: { [I<--xml>] I<xml> | I<--domain> I<domain-name-or-id-or-uuid> }

> I believe that's the way to describe mutually exclusive options where > one would be required; otherwise, we have the funky [[ ... ]] which > indicates mutually exclusive optional arguments. Search on "{ [" in > virsh.pod for some examples. This seems to be the first case where a > previously required positional argument can be both a positional and > optional as long as the alternative means of describing is provided. > > Keeping the I<xml> first at least indicates that it can be used > *without* the --xml, but is also the "default" when used positionally on > the command line. If not used positionally, then the --xml would be > required, e.g. if someone used "virsh domxml-to-native --xml @file > --format @format". > > Having the [--domain] indicates that in order to use it, it must have > that --domain argument because "virsh domxml-to-native qemu-argv > $domainname" would be invalid since the unqualified 2nd argument is > expected to be an <xml> file. > > Whether any of this is written down anywhere - I'm not sure - it's what > I've surmised over the few years. > > >> >> -Convert the file I<xml> in domain XML format to the native guest >> -configuration format named by I<format>. For QEMU/KVM hypervisor, >> +Convert the file I<xml> in domain XML format or existing domain to the >> +native guest configuration format named by I<format>. For QEMU/KVM >> hypervisor, >> the I<format> argument must be B<qemu-argv>. For Xen hypervisor, the >> I<format> argument may be B<xen-xm>, B<xen-xl>, or B<xen-sxpr>. For >> LXC hypervisor, the I<format> argument must be B<lxc-tools>. >> > > Convert the file I<xml> into domain XML format or convert an existing > I<--domain> to the native guest configuration format named by I<format>. > The I<xml> and I<--domain> arguments are mutually exclusive. > Yes, this sounds better, there is a slight flaw in the original hunk and this explains it more nicely. > John > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Jun 22, 2017 at 06:21:49PM -0400, John Ferlan wrote: > > > [...] > >>>>> >>>> >>>> There was no change, it is an additional variable, the original one is >>>> below. The number of differences would be the same, I believe. >>>> >>> >>> If edit the file and change "xml" to "xmlFile" and change the 3 changed >>> xml variable references things work... Like I said, nit, IDC if it's >>> changed or not... >>> >> >> My bad, I misread that, you're right. > > > In order to "close" on this, if a squash the attach patch does that work > for everyone? > > John WFM Reviewed-by: Martin Kletzander <mkletzan(a)redhat.com&gt; -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

My apologies. I didn't read the function logic. The second error was introduced by my "fix". The error with help parameter is happening. If --domain is required, I believe that moving to STRING is enough. As I said, VSH_OT_DATA requires VSH_OFLAG_REQ. See vsh.c line 746. {.name = "domain", - .type = VSH_OT_DATA, + .type = VSH_OT_STRING, .flags = VSH_OFLAG_REQ_OPT, .help = N_("domain name, id or uuid") }, {.name = "xml", - .type = VSH_OT_DATA, + .type = VSH_OT_STRING, .help = N_("xml data file to export from")

2017-06-24 17:54 GMT-03:00 Julio Faracco <jcfaracco(a)gmail.com&gt;: > Hi guys, > > I updated the source this weekend, I missed the ability of calling help. > virsh # domxml-to-native --help > NAME > domxml-to-native - Convert domain XML to native config > > SYNOPSIS > domxml-to-native <format> [<domain>] [<xml>] > > DESCRIPTION > Convert domain XML config to a native guest configuration format. > > OPTIONS > [--format] <string> target config data type format > error: internal error: bad options in command: 'domxml-to-native' > > > This is why: > --- a/tools/virsh-domain.c > +++ b/tools/virsh-domain.c > @@ -9858,11 +9858,11 @@ static const vshCmdOptDef opts_domxmltonative[] = { > }, > {.name = "domain", > .type = VSH_OT_DATA, > - .flags = VSH_OFLAG_REQ_OPT, > + .flags = VSH_OFLAG_REQ, > .help = N_("domain name, id or uuid") > }, > {.name = "xml", > - .type = VSH_OT_DATA, > + .type = VSH_OT_STRING, > .help = N_("xml data file to export from") > }, > {.name = NULL} > > VSH_OT_DATA requires VSH_OFLAG_REQ. > So, since XML is not required... > This diff fits this case. But I'm still confused. > Because I cannot check my XML files right now. > > virsh # domxml-to-native qemu-argv /home/julio/WINDOWS_7.xml > error: failed to get domain '/home/julio/WINDOWS_7.xml' > error: Domain not found: no domain with matching name > '/home/julio/WINDOWS_7.xml' > > 2017-06-23 6:38 GMT-03:00 Martin Kletzander <mkletzan(a)redhat.com&gt;: >> On Thu, Jun 22, 2017 at 06:21:49PM -0400, John Ferlan wrote: >>> >>> >>> [...] >>> >>>>>>> >>>>>> >>>>>> There was no change, it is an additional variable, the original one is >>>>>> below. The number of differences would be the same, I believe. >>>>>> >>>>> >>>>> If edit the file and change "xml" to "xmlFile" and change the 3 changed >>>>> xml variable references things work... Like I said, nit, IDC if it's >>>>> changed or not... >>>>> >>>> >>>> My bad, I misread that, you're right. >>> >>> >>> In order to "close" on this, if a squash the attach patch does that work >>> for everyone? >>> >>> John >> >> >> WFM >> >> Reviewed-by: Martin Kletzander <mkletzan(a)redhat.com&gt; >> >> -- >> libvir-list mailing list >> libvir-list(a)redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list

On 06/24/2017 08:05 PM, Julio Faracco wrote: > My apologies. I didn't read the function logic. > The second error was introduced by my "fix". > The error with help parameter is happening. > > If --domain is required, I believe that moving to STRING > is enough. As I said, VSH_OT_DATA requires > VSH_OFLAG_REQ. See vsh.c line 746. > > {.name = "domain", > - .type = VSH_OT_DATA, > + .type = VSH_OT_STRING, > .flags = VSH_OFLAG_REQ_OPT, > .help = N_("domain name, id or uuid") > }, > {.name = "xml", > - .type = VSH_OT_DATA, > + .type = VSH_OT_STRING, > .help = N_("xml data file to export from") > ugh... Could you send a patch please. I do recall at one point wondering what the downside of removing VSH_OFLAG_REQ from "xml" .flags was, no I know. Of course I probably won't remember the next time either Tks, - John

> 2017-06-24 17:54 GMT-03:00 Julio Faracco <jcfaracco(a)gmail.com&gt;: >> Hi guys, >> >> I updated the source this weekend, I missed the ability of calling help. >> virsh # domxml-to-native --help >> NAME >> domxml-to-native - Convert domain XML to native config >> >> SYNOPSIS >> domxml-to-native <format> [<domain>] [<xml>] >> >> DESCRIPTION >> Convert domain XML config to a native guest configuration format. >> >> OPTIONS >> [--format] <string> target config data type format >> error: internal error: bad options in command: 'domxml-to-native' >> >> >> This is why: >> --- a/tools/virsh-domain.c >> +++ b/tools/virsh-domain.c >> @@ -9858,11 +9858,11 @@ static const vshCmdOptDef opts_domxmltonative[] = { >> }, >> {.name = "domain", >> .type = VSH_OT_DATA, >> - .flags = VSH_OFLAG_REQ_OPT, >> + .flags = VSH_OFLAG_REQ, >> .help = N_("domain name, id or uuid") >> }, >> {.name = "xml", >> - .type = VSH_OT_DATA, >> + .type = VSH_OT_STRING, >> .help = N_("xml data file to export from") >> }, >> {.name = NULL} >> >> VSH_OT_DATA requires VSH_OFLAG_REQ. >> So, since XML is not required... >> This diff fits this case. But I'm still confused. >> Because I cannot check my XML files right now. >> >> virsh # domxml-to-native qemu-argv /home/julio/WINDOWS_7.xml >> error: failed to get domain '/home/julio/WINDOWS_7.xml' >> error: Domain not found: no domain with matching name >> '/home/julio/WINDOWS_7.xml' >> >> 2017-06-23 6:38 GMT-03:00 Martin Kletzander <mkletzan(a)redhat.com&gt;: >>> On Thu, Jun 22, 2017 at 06:21:49PM -0400, John Ferlan wrote: >>>> >>>> >>>> [...] >>>> >>>>>>>> >>>>>>> >>>>>>> There was no change, it is an additional variable, the original one is >>>>>>> below. The number of differences would be the same, I believe. >>>>>>> >>>>>> >>>>>> If edit the file and change "xml" to "xmlFile" and change the 3 changed >>>>>> xml variable references things work... Like I said, nit, IDC if it's >>>>>> changed or not... >>>>>> >>>>> >>>>> My bad, I misread that, you're right. >>>> >>>> >>>> In order to "close" on this, if a squash the attach patch does that work >>>> for everyone? >>>> >>>> John >>> >>> >>> WFM >>> >>> Reviewed-by: Martin Kletzander <mkletzan(a)redhat.com&gt; >>> >>> -- >>> libvir-list mailing list >>> libvir-list(a)redhat.com >>> https://www.redhat.com/mailman/listinfo/libvir-list

On Sun, Jun 25, 2017 at 08:33:39AM -0400, John Ferlan wrote: > > > On 06/24/2017 08:05 PM, Julio Faracco wrote: > > My apologies. I didn't read the function logic. > > The second error was introduced by my "fix". > > The error with help parameter is happening. > > > > If --domain is required, I believe that moving to STRING > > is enough. As I said, VSH_OT_DATA requires > > VSH_OFLAG_REQ. See vsh.c line 746. > > > > {.name = "domain", > > - .type = VSH_OT_DATA, > > + .type = VSH_OT_STRING, > > .flags = VSH_OFLAG_REQ_OPT, > > .help = N_("domain name, id or uuid") > > }, > > {.name = "xml", > > - .type = VSH_OT_DATA, > > + .type = VSH_OT_STRING, > > .help = N_("xml data file to export from") > > > > ugh... Could you send a patch please. I do recall at one point wondering > what the downside of removing VSH_OFLAG_REQ from "xml" .flags was, no I > know. Of course I probably won't remember the next time either > > Tks, - > > John > I think we may need to modify vshCmddefHelp() in vsh.c to really make it look like as expected. For example, in some case, the SYNOPSIS part also displayed incorrectly. SYNOPSIS domxml-to-native <format> <xml> <domain> Instead, the following is similar to what we want: SYNOPSIS- domxml-to-native <format> { [--xml] <xml> | --domain <domain> } Current implementation of vshCmddefHelp() does not handle this "complicated" logic. The question is whether we want to modify it or whether we want to spend the effort modifying it? Dan > > 2017-06-24 17:54 GMT-03:00 Julio Faracco <jcfaracco(a)gmail.com&gt;: > >> Hi guys, > >> > >> I updated the source this weekend, I missed the ability of calling help. > >> virsh # domxml-to-native --help > >> NAME > >> domxml-to-native - Convert domain XML to native config > >> > >> SYNOPSIS > >> domxml-to-native <format> [<domain>] [<xml>] > >> > >> DESCRIPTION > >> Convert domain XML config to a native guest configuration format. > >> > >> OPTIONS > >> [--format] <string> target config data type format > >> error: internal error: bad options in command: 'domxml-to-native' > >> > >> > >> This is why: > >> --- a/tools/virsh-domain.c > >> +++ b/tools/virsh-domain.c > >> @@ -9858,11 +9858,11 @@ static const vshCmdOptDef opts_domxmltonative[] = { > >> }, > >> {.name = "domain", > >> .type = VSH_OT_DATA, > >> - .flags = VSH_OFLAG_REQ_OPT, > >> + .flags = VSH_OFLAG_REQ, > >> .help = N_("domain name, id or uuid") > >> }, > >> {.name = "xml", > >> - .type = VSH_OT_DATA, > >> + .type = VSH_OT_STRING, > >> .help = N_("xml data file to export from") > >> }, > >> {.name = NULL} > >> > >> VSH_OT_DATA requires VSH_OFLAG_REQ. > >> So, since XML is not required... > >> This diff fits this case. But I'm still confused. > >> Because I cannot check my XML files right now. > >> > >> virsh # domxml-to-native qemu-argv /home/julio/WINDOWS_7.xml > >> error: failed to get domain '/home/julio/WINDOWS_7.xml' > >> error: Domain not found: no domain with matching name > >> '/home/julio/WINDOWS_7.xml' > >> > >> 2017-06-23 6:38 GMT-03:00 Martin Kletzander <mkletzan(a)redhat.com&gt;: > >>> On Thu, Jun 22, 2017 at 06:21:49PM -0400, John Ferlan wrote: > >>>> > >>>> > >>>> [...] > >>>> > >>>>>>>> > >>>>>>> > >>>>>>> There was no change, it is an additional variable, the original one is > >>>>>>> below. The number of differences would be the same, I believe. > >>>>>>> > >>>>>> > >>>>>> If edit the file and change "xml" to "xmlFile" and change the 3 changed > >>>>>> xml variable references things work... Like I said, nit, IDC if it's > >>>>>> changed or not... > >>>>>> > >>>>> > >>>>> My bad, I misread that, you're right. > >>>> > >>>> > >>>> In order to "close" on this, if a squash the attach patch does that work > >>>> for everyone? > >>>> > >>>> John > >>> > >>> > >>> WFM > >>> > >>> Reviewed-by: Martin Kletzander <mkletzan(a)redhat.com&gt; > >>> > >>> -- > >>> libvir-list mailing list > >>> libvir-list(a)redhat.com > >>> https://www.redhat.com/mailman/listinfo/libvir-list

On 06/02/2017 11:04 AM, Daniel Liu wrote: > 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 | virsh domxml-to-native /dev/stdin'. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=835476 > Signed-off-by: Daniel Liu <srwx4096(a)gmail.com&gt; > --- > tools/virsh-domain.c | 51 ++++++++++++++++++++++++++++++++++++++------------- > tools/virsh.pod | 7 ++++--- > 2 files changed, 42 insertions(+), 16 deletions(-) > > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c > index ccb514ef9..3496359c5 100644 > --- a/tools/virsh-domain.c > +++ b/tools/virsh-domain.c > @@ -9848,9 +9848,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} > @@ -9859,30 +9863,51 @@ 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 *xml = NULL; nit: no need to change from xmlFile to xml... The less differences the better ;-) > + char *xmlData = NULL; > + char *configData = NULL; nit: no need to change order from original, just add ' = NULL' is fine. > unsigned int flags = 0; > virshControlPtr priv = ctl->privData; > + virDomainPtr dom = NULL; > > if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 || > - vshCommandOptStringReq(ctl, cmd, "xml", &xmlFile) < 0) > + vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0) > return false; > > - if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0) > - return false; > + VSH_EXCLUSIVE_OPTIONS("domain", "xml"); > > - configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags); > - if (configData != NULL) { > - vshPrint(ctl, "%s", configData); > - VIR_FREE(configData); > + if (vshCommandOptBool(cmd, "domain") && > + (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))) > + return false; > + > + if (dom) { > + xmlData = virDomainGetXMLDesc(dom, flags); > + } else if (xml) { > + if (virFileReadAll(xml, VSH_MAX_XML_FILE, &xmlData) < 0) > + goto cleanup; > } else { > - ret = false; > + vshError(ctl, "%s", _("need either domain or domain XML")); > + goto cleanup; > + } > + > + if (!xmlData) { > + vshError(ctl, "%s", _("failed to retrieve XML")); > + goto cleanup; > } > > + if (!(configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags))) { > + goto cleanup; > + } else { > + vshPrint(ctl, "%s", configData); > + ret = true; > + } > + > + cleanup: > + virshDomainFree(dom); > VIR_FREE(xmlData); > + VIR_FREE(configData); > return ret; > } > > diff --git a/tools/virsh.pod b/tools/virsh.pod > index aee964689..049c2f3c7 100644 > --- a/tools/virsh.pod > +++ b/tools/virsh.pod > @@ -1433,10 +1433,11 @@ the I<format> argument must be B<qemu-argv>. For Xen hypervisor, the > I<format> argument may be B<xen-xm>, B<xen-xl>, or B<xen-sxpr>. For > LXC hypervisor, the I<format> argument must be B<lxc-tools>. > > -=item B<domxml-to-native> I<format> I<xml> > +=item B<domxml-to-native> I<format> > +{ I<--domain> I<domain-name-or-id-or-uuid> | [I<--xml>] I<xml> } This looks really odd, I think this should be { [I<xml> | [I<--domain> I<domain-name-or-id-uuid>] } I believe that's the way to describe mutually exclusive options where one would be required; otherwise, we have the funky [[ ... ]] which indicates mutually exclusive optional arguments. Search on "{ [" in virsh.pod for some examples. This seems to be the first case where a previously required positional argument can be both a positional and optional as long as the alternative means of describing is provided. Keeping the I<xml> first at least indicates that it can be used *without* the --xml, but is also the "default" when used positionally on the command line. If not used positionally, then the --xml would be required, e.g. if someone used "virsh domxml-to-native --xml @file --format @format". Having the [--domain] indicates that in order to use it, it must have that --domain argument because "virsh domxml-to-native qemu-argv $domainname" would be invalid since the unqualified 2nd argument is expected to be an <xml> file. Whether any of this is written down anywhere - I'm not sure - it's what I've surmised over the few years. > > -Convert the file I<xml> in domain XML format to the native guest > -configuration format named by I<format>. For QEMU/KVM hypervisor, > +Convert the file I<xml> in domain XML format or existing domain to the > +native guest configuration format named by I<format>. For QEMU/KVM hypervisor, > the I<format> argument must be B<qemu-argv>. For Xen hypervisor, the > I<format> argument may be B<xen-xm>, B<xen-xl>, or B<xen-sxpr>. For > LXC hypervisor, the I<format> argument must be B<lxc-tools>. > Convert the file I<xml> into domain XML format or convert an existing I<--domain> to the native guest configuration format named by I<format>. The I<xml> and I<--domain> arguments are mutually exclusive. John