Fix bug 835476[1].
virsh: add [--domain DOMAIN] option to  domxml-to-native DOMAIN COMMAND
Add support for the following syntax:
domxml-to-native <format> { [--domain DOMAIN] | [XML] }, i.e., it supports
either designating domain (domain id, uuid, or name), or path to XML domain
configuration file.

E.g.:
virsh domxml-to-native qemu-argv --domain RHEL7.3   # domain name
virsh domxml-to-native qemu-argv --domain 10        # domain id
virsh domxml-to-native qemu-argv dumped_dom.xml     # dumped xml

[1] https://bugzilla.redhat.com/show_bug.cgi?id=835476
---
 tools/virsh-domain.c | 54 ++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 44 insertions(+), 10 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 0d19d0e01..a79fd3ab2 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -9840,9 +9840,13 @@ static const vshCmdOptDef opts_domxmltonative[] = {
      .flags = VSH_OFLAG_REQ,
      .help = N_("target config data type format")
     },
+    {.name = "domain",
+     .type = VSH_OT_DATA,
+     .flags = VSH_OFLAG_REQ_OPT,
+     .help = N_("domain name, id or uuid")
+    },
     {.name = "xml",
      .type = VSH_OT_DATA,
-     .flags = VSH_OFLAG_REQ,
      .help = N_("xml data file to export from")
     },
     {.name = NULL}
@@ -9851,30 +9855,60 @@ static const vshCmdOptDef opts_domxmltonative[] = {
 static bool
 cmdDomXMLToNative(vshControl *ctl, const vshCmd *cmd)
 {
-    bool ret = true;
+    bool ret = false;
     const char *format = NULL;
-    const char *xmlFile = NULL;
-    char *configData;
-    char *xmlData;
+    const char *domain = NULL;
+    const char *xml = NULL;
+    char *xmlData = NULL;
+    char *configData = NULL;
     unsigned int flags = 0;
     virshControlPtr priv = ctl->privData;
+    virDomainPtr dom = NULL;
 
-    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
-        vshCommandOptStringReq(ctl, cmd, "xml", &xmlFile) < 0)
+    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0)
+        return false;
+
+    if (vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
+        return false;
+
+    if (vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0)
         return false;
 
-    if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0)
+    VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml);
+
+    if (domain)
+        dom = virshCommandOptDomain(ctl, cmd, &domain);
+
+    if (!dom && !xml) {
+        vshError(ctl, _("need either domain (ID, UUID, or name) or domain XML configuration file path"));
         return false;
+    }
+
+    if (dom) {
+        xmlData = virDomainGetXMLDesc(dom, flags);
+        if (xmlData == NULL)
+            goto cleanup;
+    }
+
+    if (xml) {
+        if (virFileReadAll(xml, VSH_MAX_XML_FILE, &xmlData) < 0)
+            goto cleanup;
+    }
 
     configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags);
     if (configData != NULL) {
         vshPrint(ctl, "%s", configData);
-        VIR_FREE(configData);
+        ret = true;
+        goto cleanup;
     } else {
-        ret = false;
+        vshError(ctl, _("convert from domain XML to native command failed"));
+        goto cleanup;
     }
 
+ 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

On Mon, May 15, 2017 at 04:29:48AM -0400, Daniel Liu wrote:
>Fix bug 835476[1].

It's enough to mention it below (as you did).

>virsh: add [--domain DOMAIN] option to  domxml-to-native DOMAIN COMMAND

This first line is already in the subject.

>Add support for the following syntax:
>domxml-to-native <format> { [--domain DOMAIN] | [XML] }, i.e., it supports
>either designating domain (domain id, uuid, or name), or path to XML domain
>configuration file.
>

I would reword this a little bit.  How would you feel about something
along the lines of:

  The option allows someone to run domain-to-native on already existing
  domain without the need of supplying their XML.  It is basically
  wrapper around `virsh dumpxml $dom | virsh domxml-to-native /dev/stdin`.

  Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=835476

>E.g.:
>virsh domxml-to-native qemu-argv --domain RHEL7.3   # domain name
>virsh domxml-to-native qemu-argv --domain 10        # domain id
>virsh domxml-to-native qemu-argv dumped_dom.xml     # dumped xml
>
>[1] https://bugzilla.redhat.com/show_bug.cgi?id=835476
>---
> tools/virsh-domain.c | 54 ++++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 44 insertions(+), 10 deletions(-)
>
>diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>index 0d19d0e01..a79fd3ab2 100644
>--- a/tools/virsh-domain.c
>+++ b/tools/virsh-domain.c
>@@ -9840,9 +9840,13 @@ static const vshCmdOptDef opts_domxmltonative[] = {
>      .flags = VSH_OFLAG_REQ,
>      .help = N_("target config data type format")
>     },
>+    {.name = "domain",
>+     .type = VSH_OT_DATA,
>+     .flags = VSH_OFLAG_REQ_OPT,
>+     .help = N_("domain name, id or uuid")
>+    },
>     {.name = "xml",
>      .type = VSH_OT_DATA,
>-     .flags = VSH_OFLAG_REQ,
>      .help = N_("xml data file to export from")
>     },
>     {.name = NULL}
>@@ -9851,30 +9855,60 @@ static const vshCmdOptDef opts_domxmltonative[] = {
> static bool
> cmdDomXMLToNative(vshControl *ctl, const vshCmd *cmd)
> {
>-    bool ret = true;
>+    bool ret = false;
>     const char *format = NULL;
>-    const char *xmlFile = NULL;
>-    char *configData;
>-    char *xmlData;
>+    const char *domain = NULL;
>+    const char *xml = NULL;
>+    char *xmlData = NULL;
>+    char *configData = NULL;
>     unsigned int flags = 0;
>     virshControlPtr priv = ctl->privData;
>+    virDomainPtr dom = NULL;
>
>-    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
>-        vshCommandOptStringReq(ctl, cmd, "xml", &xmlFile) < 0)

If this was already there, you could keep using it.  But that's not a
big deal for me, just some others might not like it.

>+    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0)
>+        return false;
>+
>+    if (vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
>+        return false;
>+

[1] So here you get the domain name/id/uuid ...

>+    if (vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0)
>         return false;
>
>-    if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0)
>+    VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml);
>+
>+    if (domain)
>+        dom = virshCommandOptDomain(ctl, cmd, &domain);
>+

... and here you get the object.  What do you supply as the third
parameter?  Check what the function does.  There's a leak that you will
fix by getting rid of the lines above [1].  And just supply NULL here.

Also make sure &dom is not NULL here, in case someone supplies
non-existing domain name.

>+    if (!dom && !xml) {
>+        vshError(ctl, _("need either domain (ID, UUID, or name) or domain XML configuration file path"));
>         return false;
>+    }
>+
>+    if (dom) {
>+        xmlData = virDomainGetXMLDesc(dom, flags);
>+        if (xmlData == NULL)
>+            goto cleanup;
>+    }
>+
>+    if (xml) {
>+        if (virFileReadAll(xml, VSH_MAX_XML_FILE, &xmlData) < 0)
>+            goto cleanup;
>+    }
>

This ^^ would be more readable as:

if (dom) {
   xmlData = blah;
} else if (xml) {
   readFile(asdf, &xmlData);
}

if (!xmlData) {
  vshError(ctl, "%s",
           _("Either <xml> or --domain must be suppied"));


Or something in that regard.

>     configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags);
>     if (configData != NULL) {
>         vshPrint(ctl, "%s", configData);
>-        VIR_FREE(configData);
>+        ret = true;
>+        goto cleanup;
>     } else {
>-        ret = false;
>+        vshError(ctl, _("convert from domain XML to native command failed"));
>+        goto cleanup;
>     }

Since you are changing this...  This does not look like all the other
places in libvirt.  What we do _almost_ exclusively is:

  something = func();
  if (!func) {
      handle error;
      goto somewhere;
  }

  what = should + continue(in.case->of.no(&error);

  ret = success code;
 cleanup:
  stuff to clean up;


I would not normally mention it, but since you are changing that part
anyway, it could be cleaned up so that it does what other parts of the
code do.  Or just keep it as it is.  It's small and right before the
cleanup, so it's _kinda_ OK (i.e. we don't have a rule written down in
the coding style for this).

Otherwise it's fine ;)

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

On Tue, May 16, 2017 at 11:41:55AM +0200, Martin Kletzander wrote:
> On Mon, May 15, 2017 at 04:29:48AM -0400, Daniel Liu wrote:
> > Fix bug 835476[1].
> 
> It's enough to mention it below (as you did).
> 
> > virsh: add [--domain DOMAIN] option to  domxml-to-native DOMAIN COMMAND
> 
> This first line is already in the subject.
> 
> > Add support for the following syntax:
> > domxml-to-native <format> { [--domain DOMAIN] | [XML] }, i.e., it supports
> > either designating domain (domain id, uuid, or name), or path to XML domain
> > configuration file.
> > 
> 
> I would reword this a little bit.  How would you feel about something
> along the lines of:
> 
>  The option allows someone to run domain-to-native on already existing
>  domain without the need of supplying their XML.  It is basically
>  wrapper around `virsh dumpxml $dom | virsh domxml-to-native /dev/stdin`.
> 
>  Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=835476
> 
I made changes accordingly in the version 2 of this patch.
> > E.g.:
> > virsh domxml-to-native qemu-argv --domain RHEL7.3   # domain name
> > virsh domxml-to-native qemu-argv --domain 10        # domain id
> > virsh domxml-to-native qemu-argv dumped_dom.xml     # dumped xml
> > 
> > [1] https://bugzilla.redhat.com/show_bug.cgi?id=835476
> > ---
> > tools/virsh-domain.c | 54 ++++++++++++++++++++++++++++++++++++++++++----------
> > 1 file changed, 44 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> > index 0d19d0e01..a79fd3ab2 100644
> > --- a/tools/virsh-domain.c
> > +++ b/tools/virsh-domain.c
> > @@ -9840,9 +9840,13 @@ static const vshCmdOptDef opts_domxmltonative[] = {
> >      .flags = VSH_OFLAG_REQ,
> >      .help = N_("target config data type format")
> >     },
> > +    {.name = "domain",
> > +     .type = VSH_OT_DATA,
> > +     .flags = VSH_OFLAG_REQ_OPT,
> > +     .help = N_("domain name, id or uuid")
> > +    },
> >     {.name = "xml",
> >      .type = VSH_OT_DATA,
> > -     .flags = VSH_OFLAG_REQ,
> >      .help = N_("xml data file to export from")
> >     },
> >     {.name = NULL}
> > @@ -9851,30 +9855,60 @@ static const vshCmdOptDef opts_domxmltonative[] = {
> > static bool
> > cmdDomXMLToNative(vshControl *ctl, const vshCmd *cmd)
> > {
> > -    bool ret = true;
> > +    bool ret = false;
> >     const char *format = NULL;
> > -    const char *xmlFile = NULL;
> > -    char *configData;
> > -    char *xmlData;
> > +    const char *domain = NULL;
> > +    const char *xml = NULL;
> > +    char *xmlData = NULL;
> > +    char *configData = NULL;
> >     unsigned int flags = 0;
> >     virshControlPtr priv = ctl->privData;
> > +    virDomainPtr dom = NULL;
> > 
> > -    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
> > -        vshCommandOptStringReq(ctl, cmd, "xml", &xmlFile) < 0)
> 
> If this was already there, you could keep using it.  But that's not a
> big deal for me, just some others might not like it.
> 
I kept the existing code and only added an additional check for "domain"
in v2, i.e.:

    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
        vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0 ||
        vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
        return false;
> > +    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0)
> > +        return false;
> > +
> > +    if (vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
> > +        return false;
> > +
> 
> [1] So here you get the domain name/id/uuid ...
> 
> > +    if (vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0)
> >         return false;
> > 
> > -    if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0)
> > +    VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml);
> > +
> > +    if (domain)
> > +        dom = virshCommandOptDomain(ctl, cmd, &domain);
> > +
> 
> ... and here you get the object.  What do you supply as the third
> parameter?  Check what the function does.  There's a leak that you will
> fix by getting rid of the lines above [1].  And just supply NULL here.
> 
I did not fully get it by "getting rid of the lines above [1]." But I
made the change as you suggested to use NULL as the third argument.
> Also make sure &dom is not NULL here, in case someone supplies
> non-existing domain name.
> 
> > +    if (!dom && !xml) {
> > +        vshError(ctl, _("need either domain (ID, UUID, or name) or domain XML configuration file path"));
> >         return false;
> > +    }
> > +
> > +    if (dom) {
> > +        xmlData = virDomainGetXMLDesc(dom, flags);
> > +        if (xmlData == NULL)
> > +            goto cleanup;
> > +    }
> > +
> > +    if (xml) {
> > +        if (virFileReadAll(xml, VSH_MAX_XML_FILE, &xmlData) < 0)
> > +            goto cleanup;
> > +    }
> > 
> 
> This ^^ would be more readable as:
> 
> if (dom) {
>   xmlData = blah;
> } else if (xml) {
>   readFile(asdf, &xmlData);
> }
> 
> if (!xmlData) {
>  vshError(ctl, "%s",
>           _("Either <xml> or --domain must be suppied"));
> 
> 
> Or something in that regard.
> 
Yep, I changed it accordingly.
> >     configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags);
> >     if (configData != NULL) {
> >         vshPrint(ctl, "%s", configData);
> > -        VIR_FREE(configData);
> > +        ret = true;
> > +        goto cleanup;
> >     } else {
> > -        ret = false;
> > +        vshError(ctl, _("convert from domain XML to native command failed"));
> > +        goto cleanup;
> >     }
> 
> Since you are changing this...  This does not look like all the other
> places in libvirt.  What we do _almost_ exclusively is:
> 
>  something = func();
>  if (!func) {
>      handle error;
>      goto somewhere;
>  }
> 
>  what = should + continue(in.case->of.no(&error);
> 
>  ret = success code;
> cleanup:
>  stuff to clean up;
> 
> 
> I would not normally mention it, but since you are changing that part
> anyway, it could be cleaned up so that it does what other parts of the
> code do.  Or just keep it as it is.  It's small and right before the
> cleanup, so it's _kinda_ OK (i.e. we don't have a rule written down in
> the coding style for this).
> 
> Otherwise it's fine ;)
> 
> Martin

I changed it to something like:
if (!(configData = virConnectDomainXMLToNative())) {
        return false;
        goto cleanup;
} else {
       vshPrint();
}

Hopefully I am following the libvirt convention here. Let me know if
there is anything more needs to be changed.

Thank you very much,

Dan

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

On Mon, May 29, 2017 at 02:25:03PM -0400, Dan wrote:
>On Tue, May 16, 2017 at 11:41:55AM +0200, Martin Kletzander wrote:
>> On Mon, May 15, 2017 at 04:29:48AM -0400, Daniel Liu wrote:
>> > Fix bug 835476[1].
>>
>> It's enough to mention it below (as you did).
>>
>> > virsh: add [--domain DOMAIN] option to  domxml-to-native DOMAIN COMMAND
>>
>> This first line is already in the subject.
>>
>> > Add support for the following syntax:
>> > domxml-to-native <format> { [--domain DOMAIN] | [XML] }, i.e., it supports
>> > either designating domain (domain id, uuid, or name), or path to XML domain
>> > configuration file.
>> >
>>
>> I would reword this a little bit.  How would you feel about something
>> along the lines of:
>>
>>  The option allows someone to run domain-to-native on already existing
>>  domain without the need of supplying their XML.  It is basically
>>  wrapper around `virsh dumpxml $dom | virsh domxml-to-native /dev/stdin`.
>>
>>  Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=835476
>>
>I made changes accordingly in the version 2 of this patch.
>> > E.g.:
>> > virsh domxml-to-native qemu-argv --domain RHEL7.3   # domain name
>> > virsh domxml-to-native qemu-argv --domain 10        # domain id
>> > virsh domxml-to-native qemu-argv dumped_dom.xml     # dumped xml
>> >
>> > [1] https://bugzilla.redhat.com/show_bug.cgi?id=835476
>> > ---
>> > tools/virsh-domain.c | 54 ++++++++++++++++++++++++++++++++++++++++++----------
>> > 1 file changed, 44 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>> > index 0d19d0e01..a79fd3ab2 100644
>> > --- a/tools/virsh-domain.c
>> > +++ b/tools/virsh-domain.c
>> > @@ -9840,9 +9840,13 @@ static const vshCmdOptDef opts_domxmltonative[] = {
>> >      .flags = VSH_OFLAG_REQ,
>> >      .help = N_("target config data type format")
>> >     },
>> > +    {.name = "domain",
>> > +     .type = VSH_OT_DATA,
>> > +     .flags = VSH_OFLAG_REQ_OPT,
>> > +     .help = N_("domain name, id or uuid")
>> > +    },
>> >     {.name = "xml",
>> >      .type = VSH_OT_DATA,
>> > -     .flags = VSH_OFLAG_REQ,
>> >      .help = N_("xml data file to export from")
>> >     },
>> >     {.name = NULL}
>> > @@ -9851,30 +9855,60 @@ static const vshCmdOptDef opts_domxmltonative[] = {
>> > static bool
>> > cmdDomXMLToNative(vshControl *ctl, const vshCmd *cmd)
>> > {
>> > -    bool ret = true;
>> > +    bool ret = false;
>> >     const char *format = NULL;
>> > -    const char *xmlFile = NULL;
>> > -    char *configData;
>> > -    char *xmlData;
>> > +    const char *domain = NULL;
>> > +    const char *xml = NULL;
>> > +    char *xmlData = NULL;
>> > +    char *configData = NULL;
>> >     unsigned int flags = 0;
>> >     virshControlPtr priv = ctl->privData;
>> > +    virDomainPtr dom = NULL;
>> >
>> > -    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
>> > -        vshCommandOptStringReq(ctl, cmd, "xml", &xmlFile) < 0)
>>
>> If this was already there, you could keep using it.  But that's not a
>> big deal for me, just some others might not like it.
>>
>I kept the existing code and only added an additional check for "domain"
>in v2, i.e.:
>
>    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
>        vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0 ||
>        vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
>        return false;
>> > +    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0)
>> > +        return false;
>> > +
>> > +    if (vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
>> > +        return false;
>> > +
>>
>> [1] So here you get the domain name/id/uuid ...
>>
>> > +    if (vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0)
>> >         return false;
>> >
>> > -    if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0)
>> > +    VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml);
>> > +
>> > +    if (domain)
>> > +        dom = virshCommandOptDomain(ctl, cmd, &domain);
>> > +
>>
>> ... and here you get the object.  What do you supply as the third
>> parameter?  Check what the function does.  There's a leak that you will
>> fix by getting rid of the lines above [1].  And just supply NULL here.
>>
>I did not fully get it by "getting rid of the lines above [1]." But I

I meant the three lines before the [1] (I used that number in square
brackets as a link.  That's why I said "Check what the function does".
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, May 29, 2017 at 08:53:18PM +0200, Martin Kletzander wrote:
> On Mon, May 29, 2017 at 02:25:03PM -0400, Dan wrote:
> > On Tue, May 16, 2017 at 11:41:55AM +0200, Martin Kletzander wrote:
> > > On Mon, May 15, 2017 at 04:29:48AM -0400, Daniel Liu wrote:
> > > > Fix bug 835476[1].
> > > 
> > > It's enough to mention it below (as you did).
> > > 
> > > > virsh: add [--domain DOMAIN] option to  domxml-to-native DOMAIN COMMAND
> > > 
> > > This first line is already in the subject.
> > > 
> > > > Add support for the following syntax:
> > > > domxml-to-native <format> { [--domain DOMAIN] | [XML] }, i.e., it supports
> > > > either designating domain (domain id, uuid, or name), or path to XML domain
> > > > configuration file.
> > > >
> > > 
> > > I would reword this a little bit.  How would you feel about something
> > > along the lines of:
> > > 
> > >  The option allows someone to run domain-to-native on already existing
> > >  domain without the need of supplying their XML.  It is basically
> > >  wrapper around `virsh dumpxml $dom | virsh domxml-to-native /dev/stdin`.
> > > 
> > >  Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=835476
> > > 
> > I made changes accordingly in the version 2 of this patch.
> > > > E.g.:
> > > > virsh domxml-to-native qemu-argv --domain RHEL7.3   # domain name
> > > > virsh domxml-to-native qemu-argv --domain 10        # domain id
> > > > virsh domxml-to-native qemu-argv dumped_dom.xml     # dumped xml
> > > >
> > > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=835476
> > > > ---
> > > > tools/virsh-domain.c | 54 ++++++++++++++++++++++++++++++++++++++++++----------
> > > > 1 file changed, 44 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> > > > index 0d19d0e01..a79fd3ab2 100644
> > > > --- a/tools/virsh-domain.c
> > > > +++ b/tools/virsh-domain.c
> > > > @@ -9840,9 +9840,13 @@ static const vshCmdOptDef opts_domxmltonative[] = {
> > > >      .flags = VSH_OFLAG_REQ,
> > > >      .help = N_("target config data type format")
> > > >     },
> > > > +    {.name = "domain",
> > > > +     .type = VSH_OT_DATA,
> > > > +     .flags = VSH_OFLAG_REQ_OPT,
> > > > +     .help = N_("domain name, id or uuid")
> > > > +    },
> > > >     {.name = "xml",
> > > >      .type = VSH_OT_DATA,
> > > > -     .flags = VSH_OFLAG_REQ,
> > > >      .help = N_("xml data file to export from")
> > > >     },
> > > >     {.name = NULL}
> > > > @@ -9851,30 +9855,60 @@ static const vshCmdOptDef opts_domxmltonative[] = {
> > > > static bool
> > > > cmdDomXMLToNative(vshControl *ctl, const vshCmd *cmd)
> > > > {
> > > > -    bool ret = true;
> > > > +    bool ret = false;
> > > >     const char *format = NULL;
> > > > -    const char *xmlFile = NULL;
> > > > -    char *configData;
> > > > -    char *xmlData;
> > > > +    const char *domain = NULL;
> > > > +    const char *xml = NULL;
> > > > +    char *xmlData = NULL;
> > > > +    char *configData = NULL;
> > > >     unsigned int flags = 0;
> > > >     virshControlPtr priv = ctl->privData;
> > > > +    virDomainPtr dom = NULL;
> > > >
> > > > -    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
> > > > -        vshCommandOptStringReq(ctl, cmd, "xml", &xmlFile) < 0)
> > > 
> > > If this was already there, you could keep using it.  But that's not a
> > > big deal for me, just some others might not like it.
> > > 
> > I kept the existing code and only added an additional check for "domain"
> > in v2, i.e.:
> > 
> >    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
> >        vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0 ||
> >        vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
> >        return false;
> > > > +    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0)
> > > > +        return false;
> > > > +
> > > > +    if (vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
> > > > +        return false;
> > > > +
> > > 
> > > [1] So here you get the domain name/id/uuid ...
> > > 
> > > > +    if (vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0)
> > > >         return false;
> > > >
> > > > -    if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0)
> > > > +    VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml);
> > > > +
> > > > +    if (domain)
> > > > +        dom = virshCommandOptDomain(ctl, cmd, &domain);
> > > > +
> > > 
> > > ... and here you get the object.  What do you supply as the third
> > > parameter?  Check what the function does.  There's a leak that you will
> > > fix by getting rid of the lines above [1].  And just supply NULL here.
> > > 
> > I did not fully get it by "getting rid of the lines above [1]." But I
> 
> I meant the three lines before the [1] (I used that number in square
> brackets as a link.  That's why I said "Check what the function does".

So I deleted the line checking "domain" leaving origin conditional like
as it was:

 if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
        vshCommandOptStringReq(ctl, cmd, "xml", &xmlFile) < 0)

However, this change would reulst in error when supplying "domain" as the
string of domain needs to be accquired, which was achieved by calling
vshCommandOptStringReq(ctl, cmd, "domain", &domain).

So far, the exception handeling and normal return are the following:


$ virsh -c qemu:///system list --all
 Id    Name                           State
----------------------------------------------------
 2     ubuntu1404                     running
 -     RHEL7.3                        shut off

$ virsh -c qemu:///system domxml-to-native
error: command 'domxml-to-native' requires <format> option

$ virsh -c qemu:///system domxml-to-native qemu-argv
error: need either domain (ID, UUID, or name) or domain XML configuration file path

$ virsh -c qemu:///system domxml-to-native qemu-argv 2
error: Failed to open file '2': No such file or directory

$ virsh -c qemu:///system domxml-to-native qemu-argv --domain 2 => OK

$ virsh -c qemu:///system domxml-to-native qemu-argv dl_dumpxml170422.xml  => OK

$ virsh -c qemu:///system domxml-to-native qemu-argv dl_dumpxml170529.xml # Wrong XML
error: convert from domain XML to native command failed
error: (domain_definition):111: expected '>'
</domain>
-------^

$ virsh -c qemu:///system domxml-to-native qemu-argv --xml dl_dumpxml170422.xml => OK

$ virsh -c qemu:///system domxml-to-native qemu-argv --xml dl_dumpxml170422.xml --domain
error: expected syntax: --domain <string>

$ virsh -c qemu:///system domxml-to-native qemu-argv --xml dl_dumpxml170422.xml --domain 3
error: Options --domain and --xml are mutually exclusive

$ virsh -c qemu:///system domxml-to-native qemu-argv --domain 3                             
error: failed to get domain '3'
error: Domain not found: no domain with matching name '3'

Best,

Dan

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

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

2017-05-29 16:40 GMT-03:00 Dan <srwx4096@gmail.com>:
> On Mon, May 29, 2017 at 08:53:18PM +0200, Martin Kletzander wrote:
>> On Mon, May 29, 2017 at 02:25:03PM -0400, Dan wrote:
>> > On Tue, May 16, 2017 at 11:41:55AM +0200, Martin Kletzander wrote:
>> > > On Mon, May 15, 2017 at 04:29:48AM -0400, Daniel Liu wrote:
>> > > > Fix bug 835476[1].
>> > >
>> > > It's enough to mention it below (as you did).
>> > >
>> > > > virsh: add [--domain DOMAIN] option to  domxml-to-native DOMAIN COMMAND
>> > >
>> > > This first line is already in the subject.
>> > >
>> > > > Add support for the following syntax:
>> > > > domxml-to-native <format> { [--domain DOMAIN] | [XML] }, i.e., it supports
>> > > > either designating domain (domain id, uuid, or name), or path to XML domain
>> > > > configuration file.
>> > > >
>> > >
>> > > I would reword this a little bit.  How would you feel about something
>> > > along the lines of:
>> > >
>> > >  The option allows someone to run domain-to-native on already existing
>> > >  domain without the need of supplying their XML.  It is basically
>> > >  wrapper around `virsh dumpxml $dom | virsh domxml-to-native /dev/stdin`.
>> > >
>> > >  Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=835476
>> > >
>> > I made changes accordingly in the version 2 of this patch.
>> > > > E.g.:
>> > > > virsh domxml-to-native qemu-argv --domain RHEL7.3   # domain name
>> > > > virsh domxml-to-native qemu-argv --domain 10        # domain id
>> > > > virsh domxml-to-native qemu-argv dumped_dom.xml     # dumped xml
>> > > >
>> > > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=835476
>> > > > ---
>> > > > tools/virsh-domain.c | 54 ++++++++++++++++++++++++++++++++++++++++++----------
>> > > > 1 file changed, 44 insertions(+), 10 deletions(-)
>> > > >
>> > > > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>> > > > index 0d19d0e01..a79fd3ab2 100644
>> > > > --- a/tools/virsh-domain.c
>> > > > +++ b/tools/virsh-domain.c
>> > > > @@ -9840,9 +9840,13 @@ static const vshCmdOptDef opts_domxmltonative[] = {
>> > > >      .flags = VSH_OFLAG_REQ,
>> > > >      .help = N_("target config data type format")
>> > > >     },
>> > > > +    {.name = "domain",
>> > > > +     .type = VSH_OT_DATA,
>> > > > +     .flags = VSH_OFLAG_REQ_OPT,
>> > > > +     .help = N_("domain name, id or uuid")
>> > > > +    },
>> > > >     {.name = "xml",
>> > > >      .type = VSH_OT_DATA,
>> > > > -     .flags = VSH_OFLAG_REQ,
>> > > >      .help = N_("xml data file to export from")
>> > > >     },
>> > > >     {.name = NULL}
>> > > > @@ -9851,30 +9855,60 @@ static const vshCmdOptDef opts_domxmltonative[] = {
>> > > > static bool
>> > > > cmdDomXMLToNative(vshControl *ctl, const vshCmd *cmd)
>> > > > {
>> > > > -    bool ret = true;
>> > > > +    bool ret = false;
>> > > >     const char *format = NULL;
>> > > > -    const char *xmlFile = NULL;
>> > > > -    char *configData;
>> > > > -    char *xmlData;
>> > > > +    const char *domain = NULL;
>> > > > +    const char *xml = NULL;
>> > > > +    char *xmlData = NULL;
>> > > > +    char *configData = NULL;
>> > > >     unsigned int flags = 0;
>> > > >     virshControlPtr priv = ctl->privData;
>> > > > +    virDomainPtr dom = NULL;
>> > > >
>> > > > -    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
>> > > > -        vshCommandOptStringReq(ctl, cmd, "xml", &xmlFile) < 0)
>> > >
>> > > If this was already there, you could keep using it.  But that's not a
>> > > big deal for me, just some others might not like it.
>> > >
>> > I kept the existing code and only added an additional check for "domain"
>> > in v2, i.e.:
>> >
>> >    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
>> >        vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0 ||
>> >        vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
>> >        return false;
>> > > > +    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0)
>> > > > +        return false;
>> > > > +
>> > > > +    if (vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
>> > > > +        return false;
>> > > > +
>> > >
>> > > [1] So here you get the domain name/id/uuid ...
>> > >
>> > > > +    if (vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0)
>> > > >         return false;
>> > > >
>> > > > -    if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0)
>> > > > +    VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml);
>> > > > +
>> > > > +    if (domain)
>> > > > +        dom = virshCommandOptDomain(ctl, cmd, &domain);
>> > > > +
>> > >
>> > > ... and here you get the object.  What do you supply as the third
>> > > parameter?  Check what the function does.  There's a leak that you will
>> > > fix by getting rid of the lines above [1].  And just supply NULL here.
>> > >
>> > I did not fully get it by "getting rid of the lines above [1]." But I
>>
>> I meant the three lines before the [1] (I used that number in square
>> brackets as a link.  That's why I said "Check what the function does".
>
> So I deleted the line checking "domain" leaving origin conditional like
> as it was:
>
>  if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
>         vshCommandOptStringReq(ctl, cmd, "xml", &xmlFile) < 0)
>
> However, this change would reulst in error when supplying "domain" as the
> string of domain needs to be accquired, which was achieved by calling
> vshCommandOptStringReq(ctl, cmd, "domain", &domain).
>
> So far, the exception handeling and normal return are the following:
>
>
> $ virsh -c qemu:///system list --all
>  Id    Name                           State
> ----------------------------------------------------
>  2     ubuntu1404                     running
>  -     RHEL7.3                        shut off
>
> $ virsh -c qemu:///system domxml-to-native
> error: command 'domxml-to-native' requires <format> option
>
> $ virsh -c qemu:///system domxml-to-native qemu-argv
> error: need either domain (ID, UUID, or name) or domain XML configuration file path
>
> $ virsh -c qemu:///system domxml-to-native qemu-argv 2
> error: Failed to open file '2': No such file or directory
>
> $ virsh -c qemu:///system domxml-to-native qemu-argv --domain 2 => OK
>
> $ virsh -c qemu:///system domxml-to-native qemu-argv dl_dumpxml170422.xml  => OK
>
> $ virsh -c qemu:///system domxml-to-native qemu-argv dl_dumpxml170529.xml # Wrong XML
> error: convert from domain XML to native command failed
> error: (domain_definition):111: expected '>'
> </domain>
> -------^
>
> $ virsh -c qemu:///system domxml-to-native qemu-argv --xml dl_dumpxml170422.xml => OK
>
> $ virsh -c qemu:///system domxml-to-native qemu-argv --xml dl_dumpxml170422.xml --domain
> error: expected syntax: --domain <string>
>
> $ virsh -c qemu:///system domxml-to-native qemu-argv --xml dl_dumpxml170422.xml --domain 3
> error: Options --domain and --xml are mutually exclusive
>
> $ virsh -c qemu:///system domxml-to-native qemu-argv --domain 3
> error: failed to get domain '3'
> error: Domain not found: no domain with matching name '3'
>
> Best,
>
> Dan
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

On Mon, May 29, 2017 at 05:50:46PM -0300, Julio Faracco wrote:
> It would be nice to include the changes into documentation, wouldn't it?
> tools/virsh.pod has an entry for example.
> 
Many thanks for pointing it out. I modified man page by editing virsh.pod
in v3 patch.

Dan
> 2017-05-29 16:40 GMT-03:00 Dan <srwx4096@gmail.com>:
> > On Mon, May 29, 2017 at 08:53:18PM +0200, Martin Kletzander wrote:
> >> On Mon, May 29, 2017 at 02:25:03PM -0400, Dan wrote:
> >> > On Tue, May 16, 2017 at 11:41:55AM +0200, Martin Kletzander wrote:
> >> > > On Mon, May 15, 2017 at 04:29:48AM -0400, Daniel Liu wrote:
> >> > > > Fix bug 835476[1].
> >> > >
> >> > > It's enough to mention it below (as you did).
> >> > >
> >> > > > virsh: add [--domain DOMAIN] option to  domxml-to-native DOMAIN COMMAND
> >> > >
> >> > > This first line is already in the subject.
> >> > >
> >> > > > Add support for the following syntax:
> >> > > > domxml-to-native <format> { [--domain DOMAIN] | [XML] }, i.e., it supports
> >> > > > either designating domain (domain id, uuid, or name), or path to XML domain
> >> > > > configuration file.
> >> > > >
> >> > >
> >> > > I would reword this a little bit.  How would you feel about something
> >> > > along the lines of:
> >> > >
> >> > >  The option allows someone to run domain-to-native on already existing
> >> > >  domain without the need of supplying their XML.  It is basically
> >> > >  wrapper around `virsh dumpxml $dom | virsh domxml-to-native /dev/stdin`.
> >> > >
> >> > >  Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=835476
> >> > >
> >> > I made changes accordingly in the version 2 of this patch.
> >> > > > E.g.:
> >> > > > virsh domxml-to-native qemu-argv --domain RHEL7.3   # domain name
> >> > > > virsh domxml-to-native qemu-argv --domain 10        # domain id
> >> > > > virsh domxml-to-native qemu-argv dumped_dom.xml     # dumped xml
> >> > > >
> >> > > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=835476
> >> > > > ---
> >> > > > tools/virsh-domain.c | 54 ++++++++++++++++++++++++++++++++++++++++++----------
> >> > > > 1 file changed, 44 insertions(+), 10 deletions(-)
> >> > > >
> >> > > > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> >> > > > index 0d19d0e01..a79fd3ab2 100644
> >> > > > --- a/tools/virsh-domain.c
> >> > > > +++ b/tools/virsh-domain.c
> >> > > > @@ -9840,9 +9840,13 @@ static const vshCmdOptDef opts_domxmltonative[] = {
> >> > > >      .flags = VSH_OFLAG_REQ,
> >> > > >      .help = N_("target config data type format")
> >> > > >     },
> >> > > > +    {.name = "domain",
> >> > > > +     .type = VSH_OT_DATA,
> >> > > > +     .flags = VSH_OFLAG_REQ_OPT,
> >> > > > +     .help = N_("domain name, id or uuid")
> >> > > > +    },
> >> > > >     {.name = "xml",
> >> > > >      .type = VSH_OT_DATA,
> >> > > > -     .flags = VSH_OFLAG_REQ,
> >> > > >      .help = N_("xml data file to export from")
> >> > > >     },
> >> > > >     {.name = NULL}
> >> > > > @@ -9851,30 +9855,60 @@ static const vshCmdOptDef opts_domxmltonative[] = {
> >> > > > static bool
> >> > > > cmdDomXMLToNative(vshControl *ctl, const vshCmd *cmd)
> >> > > > {
> >> > > > -    bool ret = true;
> >> > > > +    bool ret = false;
> >> > > >     const char *format = NULL;
> >> > > > -    const char *xmlFile = NULL;
> >> > > > -    char *configData;
> >> > > > -    char *xmlData;
> >> > > > +    const char *domain = NULL;
> >> > > > +    const char *xml = NULL;
> >> > > > +    char *xmlData = NULL;
> >> > > > +    char *configData = NULL;
> >> > > >     unsigned int flags = 0;
> >> > > >     virshControlPtr priv = ctl->privData;
> >> > > > +    virDomainPtr dom = NULL;
> >> > > >
> >> > > > -    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
> >> > > > -        vshCommandOptStringReq(ctl, cmd, "xml", &xmlFile) < 0)
> >> > >
> >> > > If this was already there, you could keep using it.  But that's not a
> >> > > big deal for me, just some others might not like it.
> >> > >
> >> > I kept the existing code and only added an additional check for "domain"
> >> > in v2, i.e.:
> >> >
> >> >    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
> >> >        vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0 ||
> >> >        vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
> >> >        return false;
> >> > > > +    if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0)
> >> > > > +        return false;
> >> > > > +
> >> > > > +    if (vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)
> >> > > > +        return false;
> >> > > > +
> >> > >
> >> > > [1] So here you get the domain name/id/uuid ...
> >> > >
> >> > > > +    if (vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0)
> >> > > >         return false;
> >> > > >
> >> > > > -    if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0)
> >> > > > +    VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml);
> >> > > > +
> >> > > > +    if (domain)
> >> > > > +        dom = virshCommandOptDomain(ctl, cmd, &domain);
> >> > > > +
> >> > >
> >> > > ... and here you get the object.  What do you supply as the third
> >> > > parameter?  Check what the function does.  There's a leak that you will
> >> > > fix by getting rid of the lines above [1].  And just supply NULL here.
> >> > >
> >> > I did not fully get it by "getting rid of the lines above [1]." But I
> >>
> >> I meant the three lines before the [1] (I used that number in square
> >> brackets as a link.  That's why I said "Check what the function does".
> >
> > So I deleted the line checking "domain" leaving origin conditional like
> > as it was:
> >
> >  if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 ||
> >         vshCommandOptStringReq(ctl, cmd, "xml", &xmlFile) < 0)
> >
> > However, this change would reulst in error when supplying "domain" as the
> > string of domain needs to be accquired, which was achieved by calling
> > vshCommandOptStringReq(ctl, cmd, "domain", &domain).
> >
> > So far, the exception handeling and normal return are the following:
> >
> >
> > $ virsh -c qemu:///system list --all
> >  Id    Name                           State
> > ----------------------------------------------------
> >  2     ubuntu1404                     running
> >  -     RHEL7.3                        shut off
> >
> > $ virsh -c qemu:///system domxml-to-native
> > error: command 'domxml-to-native' requires <format> option
> >
> > $ virsh -c qemu:///system domxml-to-native qemu-argv
> > error: need either domain (ID, UUID, or name) or domain XML configuration file path
> >
> > $ virsh -c qemu:///system domxml-to-native qemu-argv 2
> > error: Failed to open file '2': No such file or directory
> >
> > $ virsh -c qemu:///system domxml-to-native qemu-argv --domain 2 => OK
> >
> > $ virsh -c qemu:///system domxml-to-native qemu-argv dl_dumpxml170422.xml  => OK
> >
> > $ virsh -c qemu:///system domxml-to-native qemu-argv dl_dumpxml170529.xml # Wrong XML
> > error: convert from domain XML to native command failed
> > error: (domain_definition):111: expected '>'
> > </domain>
> > -------^
> >
> > $ virsh -c qemu:///system domxml-to-native qemu-argv --xml dl_dumpxml170422.xml => OK
> >
> > $ virsh -c qemu:///system domxml-to-native qemu-argv --xml dl_dumpxml170422.xml --domain
> > error: expected syntax: --domain <string>
> >
> > $ virsh -c qemu:///system domxml-to-native qemu-argv --xml dl_dumpxml170422.xml --domain 3
> > error: Options --domain and --xml are mutually exclusive
> >
> > $ virsh -c qemu:///system domxml-to-native qemu-argv --domain 3
> > error: failed to get domain '3'
> > error: Domain not found: no domain with matching name '3'
> >
> > Best,
> >
> > Dan
> >
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list

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