[libvirt] [PATCH v2] virsh: add [--domain DOMAIN] option to domxml-to-native DOMAIN COMMAND

Daniel Liu posted 1 patch 6 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20170529180853.4341-1-srwx4096@gmail.com
There is a newer version of this series
tools/virsh-domain.c | 52 ++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 40 insertions(+), 12 deletions(-)
[libvirt] [PATCH v2] virsh: add [--domain DOMAIN] option to domxml-to-native DOMAIN COMMAND
Posted by Daniel Liu 6 years, 10 months ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] virsh: add [--domain DOMAIN] option to domxml-to-native DOMAIN COMMAND
Posted by Peter Krempa 6 years, 10 months ago
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;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] virsh: add [--domain DOMAIN] option to domxml-to-native DOMAIN COMMAND
Posted by Dan 6 years, 10 months ago
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;


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] virsh: add [--domain DOMAIN] option to domxml-to-native DOMAIN COMMAND
Posted by Martin Kletzander 6 years, 10 months ago
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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] virsh: add [--domain DOMAIN] option to domxml-to-native DOMAIN COMMAND
Posted by Dan 6 years, 10 months ago
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;


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] virsh: add [--domain DOMAIN] option to domxml-to-native DOMAIN COMMAND
Posted by Martin Kletzander 6 years, 10 months ago
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;
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] virsh: add [--domain DOMAIN] option to domxml-to-native DOMAIN COMMAND
Posted by Dan 6 years, 9 months ago
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;
> > 
> > 


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] virsh: add [--domain DOMAIN] option to domxml-to-native DOMAIN COMMAND
Posted by Martin Kletzander 6 years, 10 months ago
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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] virsh: add [--domain DOMAIN] option to domxml-to-native DOMAIN COMMAND
Posted by Dan 6 years, 10 months ago
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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list