[libvirt] [PATCH v2] virsh: add [--domain DOMAIN] option to domxml-to-native DOMAIN COMMAND
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 --- tools/virsh-domain.c | 52 ++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 12 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ccb514ef9..929f9c896 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,54 @@ 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) + vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0 || + vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0) return false; - if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0) + VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml); + + if (domain) { + if (!(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; + } + + if (!xmlData) { + vshError(ctl, "%s", + _("need either domain (ID, UUID, or name) or domain XML configuration file path")); return false; + } - configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags); - if (configData != NULL) { - vshPrint(ctl, "%s", configData); - VIR_FREE(configData); + if (!(configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags))) { + vshError(ctl, "%s", + _("convert from domain XML to native command failed")); + goto cleanup; } else { - ret = false; + vshPrint(ctl, "%s", configData); + ret = true; } + cleanup: + virshDomainFree(dom); VIR_FREE(xmlData); + VIR_FREE(configData); return ret; } -- 2.13.0
On Mon, May 29, 2017 at 14:08:53 -0400, 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 --- tools/virsh-domain.c | 52 ++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 12 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ccb514ef9..929f9c896 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c
[...]
@@ -9859,30 +9863,54 @@ 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) + vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0 || + vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
You don't need this variable after the check below, so it's pointless to extract it.
return false;
- if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0) + VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml);
This macro is documented to work only on booleans, please don't use it on pointers.
+ + if (domain) { + if (!(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; + } + + if (!xmlData) { + vshError(ctl, "%s", + _("need either domain (ID, UUID, or name) or domain XML configuration file path"));
This line is very long.
return false;
You'll leak 'dom' here if it was looked up, but getting of the XML failed. Also the message is kind of wrong in that case, since you've provided a valid name, but the XML could not be requested
+ }
- configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags); - if (configData != NULL) { - vshPrint(ctl, "%s", configData); - VIR_FREE(configData); + if (!(configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags))) { + vshError(ctl, "%s", + _("convert from domain XML to native command failed"));
For 'xen' the output is not really a command, so this message also is not very good.
+ goto cleanup; } else { - ret = false; + vshPrint(ctl, "%s", configData); + ret = true;
On Tue, May 30, 2017 at 08:45:37AM +0200, Peter Krempa wrote:
On Mon, May 29, 2017 at 14:08:53 -0400, 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 --- tools/virsh-domain.c | 52 ++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 12 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ccb514ef9..929f9c896 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c
[...]
@@ -9859,30 +9863,54 @@ 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) + vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0 || + vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
You don't need this variable after the check below, so it's pointless to extract it.
Right, I finally realized it this morning and deleted this variable.
return false;
- if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0) + VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml);
This macro is documented to work only on booleans, please don't use it on pointers.
I switched to using VSH_EXCLUSIVE_OPTIONS(string1, string2) in patch v3;
+ + if (domain) { + if (!(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; + } + + if (!xmlData) { + vshError(ctl, "%s", + _("need either domain (ID, UUID, or name) or domain XML configuration file path"));
This line is very long.
return false;
You'll leak 'dom' here if it was looked up, but getting of the XML failed.
Also the message is kind of wrong in that case, since you've provided a valid name, but the XML could not be requested
Thanks a lot for the review; I was being careless.
+ }
- configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags); - if (configData != NULL) { - vshPrint(ctl, "%s", configData); - VIR_FREE(configData); + if (!(configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags))) { + vshError(ctl, "%s", + _("convert from domain XML to native command failed"));
For 'xen' the output is not really a command, so this message also is not very good.
I have never used/tested xen. And actually I do not know where to start setting it up except thinking about googling. Could you elaborate a bit more? Thanks, Dan
+ goto cleanup; } else { - ret = false; + vshPrint(ctl, "%s", configData); + ret = true;
On Wed, May 31, 2017 at 11:27:42AM -0400, Dan wrote:
On Tue, May 30, 2017 at 08:45:37AM +0200, Peter Krempa wrote:
On Mon, May 29, 2017 at 14:08:53 -0400, 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 --- tools/virsh-domain.c | 52 ++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 12 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ccb514ef9..929f9c896 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c
[...]
@@ -9859,30 +9863,54 @@ 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) + vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0 || + vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
You don't need this variable after the check below, so it's pointless to extract it.
Right, I finally realized it this morning and deleted this variable.
return false;
- if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0) + VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml);
This macro is documented to work only on booleans, please don't use it on pointers.
I switched to using VSH_EXCLUSIVE_OPTIONS(string1, string2) in patch v3;
+ + if (domain) { + if (!(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; + } + + if (!xmlData) { + vshError(ctl, "%s", + _("need either domain (ID, UUID, or name) or domain XML configuration file path"));
This line is very long.
return false;
You'll leak 'dom' here if it was looked up, but getting of the XML failed.
Also the message is kind of wrong in that case, since you've provided a valid name, but the XML could not be requested
Thanks a lot for the review; I was being careless.
+ }
- configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags); - if (configData != NULL) { - vshPrint(ctl, "%s", configData); - VIR_FREE(configData); + if (!(configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags))) { + vshError(ctl, "%s", + _("convert from domain XML to native command failed"));
For 'xen' the output is not really a command, so this message also is not very good.
I have never used/tested xen. And actually I do not know where to start setting it up except thinking about googling. Could you elaborate a bit more?
For QEMU, the native interface is a command. For Xen it is not a command (I think it's some kind of a configuration or some other definition). Basically not all formats have commands as output, that's it, nothing complicated. There was no error before and it's still fine, so you could've just skipped the error entirely, but that doesn't really matter.
On Tue, May 30, 2017 at 08:45:37AM +0200, Peter Krempa wrote:
On Mon, May 29, 2017 at 14:08:53 -0400, 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 --- tools/virsh-domain.c | 52 ++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 12 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ccb514ef9..929f9c896 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c
[...]
@@ -9859,30 +9863,54 @@ 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) + vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0 || + vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
You don't need this variable after the check below, so it's pointless to extract it.
return false;
- if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0) + VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml);
This macro is documented to work only on booleans, please don't use it on pointers.
+ + if (domain) { + if (!(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; + } + + if (!xmlData) { + vshError(ctl, "%s", + _("need either domain (ID, UUID, or name) or domain XML configuration file path"));
This line is very long.
return false;
You'll leak 'dom' here if it was looked up, but getting of the XML failed.
Also the message is kind of wrong in that case, since you've provided a valid name, but the XML could not be requested
+ }
- configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags); - if (configData != NULL) { - vshPrint(ctl, "%s", configData); - VIR_FREE(configData); + if (!(configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags))) { + vshError(ctl, "%s", + _("convert from domain XML to native command failed"));
For 'xen' the output is not really a command, so this message also is not very good.
I found out it is actually XM config files from the domxml-to-native command https://libvirt.org/drvxen.html#xmlexport So for v4 of this patch, I am going to delete the "command" word, i.e.: vshError(ctl, "%s", _("convert from domain XML to native failed")); some test cases when this error happened: $ virsh -c qemu:///system domxml-to-native qemu-argv dl_dumpxml170529.xml error: convert from domain XML to native command failed error: (domain_definition):111: expected '>' </domain> -------^ $ echo $? 1 $ virsh -c qemu:///system domxml-to-native qemu-argv dl_test_dumpxml.xml error: convert from domain XML to native command failed error: invalid argument: could not find capabilities for arch=i686 domaintype=test Btw, for the first error, the erroneous part of my test xml was actually the first line of a domain xml dump; I deleted the character 'n' in 'domain' string: <domai type='kvm' id='1'> <name>ubuntu1404</name> <uuid>e8742104-ecac-2ed9-a9d6-e1x15319374f</uuid> i.e., the error message did not detect the true invalidity of the xml file. But this is not a big issue, right? P.S. Is there anything more I need to change for this PATCH? Thanks, Dan
} else { - ret = false; + vshPrint(ctl, "%s", configData); + ret = true;
On Fri, Jun 02, 2017 at 05:18:52AM -0400, Dan wrote:
On Tue, May 30, 2017 at 08:45:37AM +0200, Peter Krempa wrote:
On Mon, May 29, 2017 at 14:08:53 -0400, 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 --- tools/virsh-domain.c | 52 ++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 12 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ccb514ef9..929f9c896 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c
[...]
@@ -9859,30 +9863,54 @@ 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) + vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0 || + vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
You don't need this variable after the check below, so it's pointless to extract it.
return false;
- if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0) + VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml);
This macro is documented to work only on booleans, please don't use it on pointers.
+ + if (domain) { + if (!(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; + } + + if (!xmlData) { + vshError(ctl, "%s", + _("need either domain (ID, UUID, or name) or domain XML configuration file path"));
This line is very long.
return false;
You'll leak 'dom' here if it was looked up, but getting of the XML failed.
Also the message is kind of wrong in that case, since you've provided a valid name, but the XML could not be requested
+ }
- configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags); - if (configData != NULL) { - vshPrint(ctl, "%s", configData); - VIR_FREE(configData); + if (!(configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags))) { + vshError(ctl, "%s", + _("convert from domain XML to native command failed"));
For 'xen' the output is not really a command, so this message also is not very good.
I found out it is actually XM config files from the domxml-to-native command https://libvirt.org/drvxen.html#xmlexport
So for v4 of this patch, I am going to delete the "command" word, i.e.: vshError(ctl, "%s", _("convert from domain XML to native failed"));
That sentence doesn't make sense. "Conversion from domain XML to native representation failed" would make more sense, but I don't know why you are complicating it for yourself. There was no vshError() before your patch and I don't see why it would be needed.
some test cases when this error happened:
$ virsh -c qemu:///system domxml-to-native qemu-argv dl_dumpxml170529.xml error: convert from domain XML to native command failed error: (domain_definition):111: expected '>' </domain> -------^
$ echo $? 1 $ virsh -c qemu:///system domxml-to-native qemu-argv dl_test_dumpxml.xml error: convert from domain XML to native command failed error: invalid argument: could not find capabilities for arch=i686 domaintype=test
Btw, for the first error, the erroneous part of my test xml was actually the first line of a domain xml dump; I deleted the character 'n' in 'domain' string:
<domai type='kvm' id='1'> <name>ubuntu1404</name> <uuid>e8742104-ecac-2ed9-a9d6-e1x15319374f</uuid>
i.e., the error message did not detect the true invalidity of the xml file. But this is not a big issue, right?
Well, it actually did: error: (domain_definition):111: expected '>' This is the error from libxml2 forwarded through libvirt.
P.S. Is there anything more I need to change for this PATCH?
Thanks,
Dan
} else { - ret = false; + vshPrint(ctl, "%s", configData); + ret = true;
On Fri, Jun 02, 2017 at 11:33:50AM +0200, Martin Kletzander wrote:
On Fri, Jun 02, 2017 at 05:18:52AM -0400, Dan wrote:
On Tue, May 30, 2017 at 08:45:37AM +0200, Peter Krempa wrote:
On Mon, May 29, 2017 at 14:08:53 -0400, 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 --- tools/virsh-domain.c | 52 ++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 12 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ccb514ef9..929f9c896 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c
[...]
@@ -9859,30 +9863,54 @@ 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) + vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0 || + vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
You don't need this variable after the check below, so it's pointless to extract it.
return false;
- if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0) + VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml);
This macro is documented to work only on booleans, please don't use it on pointers.
+ + if (domain) { + if (!(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; + } + + if (!xmlData) { + vshError(ctl, "%s", + _("need either domain (ID, UUID, or name) or domain XML configuration file path"));
This line is very long.
return false;
You'll leak 'dom' here if it was looked up, but getting of the XML failed.
Also the message is kind of wrong in that case, since you've provided a valid name, but the XML could not be requested
+ }
- configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags); - if (configData != NULL) { - vshPrint(ctl, "%s", configData); - VIR_FREE(configData); + if (!(configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags))) { + vshError(ctl, "%s", + _("convert from domain XML to native command failed"));
For 'xen' the output is not really a command, so this message also is not very good.
I found out it is actually XM config files from the domxml-to-native command https://libvirt.org/drvxen.html#xmlexport
So for v4 of this patch, I am going to delete the "command" word, i.e.: vshError(ctl, "%s", _("convert from domain XML to native failed"));
That sentence doesn't make sense. "Conversion from domain XML to native representation failed" would make more sense, but I don't know why you are complicating it for yourself. There was no vshError() before your patch and I don't see why it would be needed.
Hey Martin, Finally, I deleted the error message print statement in patch v4. Cheers, Dan
some test cases when this error happened:
$ virsh -c qemu:///system domxml-to-native qemu-argv dl_dumpxml170529.xml error: convert from domain XML to native command failed error: (domain_definition):111: expected '>' </domain> -------^
$ echo $? 1 $ virsh -c qemu:///system domxml-to-native qemu-argv dl_test_dumpxml.xml error: convert from domain XML to native command failed error: invalid argument: could not find capabilities for arch=i686 domaintype=test
Btw, for the first error, the erroneous part of my test xml was actually the first line of a domain xml dump; I deleted the character 'n' in 'domain' string:
<domai type='kvm' id='1'> <name>ubuntu1404</name> <uuid>e8742104-ecac-2ed9-a9d6-e1x15319374f</uuid>
i.e., the error message did not detect the true invalidity of the xml file. But this is not a big issue, right?
Well, it actually did:
error: (domain_definition):111: expected '>'
This is the error from libxml2 forwarded through libvirt.
P.S. Is there anything more I need to change for this PATCH?
Thanks,
Dan
} else { - ret = false; + vshPrint(ctl, "%s", configData); + ret = true;
On Mon, May 29, 2017 at 02:08:53PM -0400, 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 --- tools/virsh-domain.c | 52 ++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 12 deletions(-)
Just few things I forgot to sent, so they remained in my drafts folder.
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ccb514ef9..929f9c896 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9859,30 +9863,54 @@ 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) + vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0 || + vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
You don't have to do this, you don't use the domain name anywhere, just the information whether it's present or not.
return false;
- if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0) + VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml); + + if (domain) { + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false;
And instead of this, you can just do: if (vshCommandOptBool("domain") && (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))) return false;
+ } + + if (dom) { + xmlData = virDomainGetXMLDesc(dom, flags);
xmlData can be NULL after this in case of an error.
+ } else if (xml) { + if (virFileReadAll(xml, VSH_MAX_XML_FILE, &xmlData) < 0) + goto cleanup; + } + + if (!xmlData) {
So this could just be else {} of the last condition and if xmlData is NULL after calling virDomainGetXMLDesc(), there should be different error message. Other things were pointed out already, I guess.
On Wed, May 31, 2017 at 02:32:51PM +0200, Martin Kletzander wrote:
On Mon, May 29, 2017 at 02:08:53PM -0400, 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 --- tools/virsh-domain.c | 52 ++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 12 deletions(-)
Just few things I forgot to sent, so they remained in my drafts folder.
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ccb514ef9..929f9c896 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9859,30 +9863,54 @@ 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) + vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0 || + vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
You don't have to do this, you don't use the domain name anywhere, just the information whether it's present or not.
return false;
- if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0) + VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml); + + if (domain) { + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false;
And instead of this, you can just do:
if (vshCommandOptBool("domain") && (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))) return false;
Oh yeah, I basically did what you suggested, it took me the entire morning to figure out those things. I wish I checked your email earlier. I was using nested-if here, but in the v3 PATCH, I took your approach.
+ } + + if (dom) { + xmlData = virDomainGetXMLDesc(dom, flags);
xmlData can be NULL after this in case of an error.
+ } else if (xml) { + if (virFileReadAll(xml, VSH_MAX_XML_FILE, &xmlData) < 0) + goto cleanup; + } + + if (!xmlData) {
So this could just be else {} of the last condition and if xmlData is NULL after calling virDomainGetXMLDesc(), there should be different error message.
Other things were pointed out already, I guess. All right thanks a lot.
Dan
participants (4)
-
Dan -
Daniel Liu
-
Martin Kletzander -
Peter Krempa