[libvirt] [PATCH] virsh: add --domain option for domain-to-native
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
On Mon, May 15, 2017 at 04:29:48AM -0400, Daniel Liu wrote:
Fix bug 835476[1].
It's enough to mention it below (as you did).
virsh: add [--domain DOMAIN] option to domxml-to-native DOMAIN COMMAND
This first line is already in the subject.
Add support for the following syntax: domxml-to-native <format> { [--domain DOMAIN] | [XML] }, i.e., it supports either designating domain (domain id, uuid, or name), or path to XML domain configuration file.
I would reword this a little bit. How would you feel about something along the lines of: The option allows someone to run domain-to-native on already existing domain without the need of supplying their XML. It is basically wrapper around `virsh dumpxml $dom | virsh domxml-to-native /dev/stdin`. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=835476
E.g.: virsh domxml-to-native qemu-argv --domain RHEL7.3 # domain name virsh domxml-to-native qemu-argv --domain 10 # domain id virsh domxml-to-native qemu-argv dumped_dom.xml # dumped xml
[1] https://bugzilla.redhat.com/show_bug.cgi?id=835476 --- tools/virsh-domain.c | 54 ++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 44 insertions(+), 10 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 0d19d0e01..a79fd3ab2 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9840,9 +9840,13 @@ static const vshCmdOptDef opts_domxmltonative[] = { .flags = VSH_OFLAG_REQ, .help = N_("target config data type format") }, + {.name = "domain", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ_OPT, + .help = N_("domain name, id or uuid") + }, {.name = "xml", .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, .help = N_("xml data file to export from") }, {.name = NULL} @@ -9851,30 +9855,60 @@ static const vshCmdOptDef opts_domxmltonative[] = { static bool cmdDomXMLToNative(vshControl *ctl, const vshCmd *cmd) { - bool ret = true; + bool ret = false; const char *format = NULL; - const char *xmlFile = NULL; - char *configData; - char *xmlData; + const char *domain = NULL; + const char *xml = NULL; + char *xmlData = NULL; + char *configData = NULL; unsigned int flags = 0; virshControlPtr priv = ctl->privData; + virDomainPtr dom = NULL;
- if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0 || - vshCommandOptStringReq(ctl, cmd, "xml", &xmlFile) < 0)
If this was already there, you could keep using it. But that's not a big deal for me, just some others might not like it.
+ if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0) + return false; + + if (vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0) + return false; +
[1] So here you get the domain name/id/uuid ...
+ if (vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0) return false;
- if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0) + VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml); + + if (domain) + dom = virshCommandOptDomain(ctl, cmd, &domain); +
... and here you get the object. What do you supply as the third parameter? Check what the function does. There's a leak that you will fix by getting rid of the lines above [1]. And just supply NULL here. Also make sure &dom is not NULL here, in case someone supplies non-existing domain name.
+ if (!dom && !xml) { + vshError(ctl, _("need either domain (ID, UUID, or name) or domain XML configuration file path")); return false; + } + + if (dom) { + xmlData = virDomainGetXMLDesc(dom, flags); + if (xmlData == NULL) + goto cleanup; + } + + if (xml) { + if (virFileReadAll(xml, VSH_MAX_XML_FILE, &xmlData) < 0) + goto cleanup; + }
This ^^ would be more readable as: if (dom) { xmlData = blah; } else if (xml) { readFile(asdf, &xmlData); } if (!xmlData) { vshError(ctl, "%s", _("Either <xml> or --domain must be suppied")); Or something in that regard.
configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags); if (configData != NULL) { vshPrint(ctl, "%s", configData); - VIR_FREE(configData); + ret = true; + goto cleanup; } else { - ret = false; + vshError(ctl, _("convert from domain XML to native command failed")); + goto cleanup; }
Since you are changing this... This does not look like all the other places in libvirt. What we do _almost_ exclusively is: something = func(); if (!func) { handle error; goto somewhere; } what = should + continue(in.case->of.no(&error); ret = success code; cleanup: stuff to clean up; I would not normally mention it, but since you are changing that part anyway, it could be cleaned up so that it does what other parts of the code do. Or just keep it as it is. It's small and right before the cleanup, so it's _kinda_ OK (i.e. we don't have a rule written down in the coding style for this). Otherwise it's fine ;) Martin
On 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
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".
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
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
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
participants (4)
-
Dan -
Daniel Liu
-
Julio Faracco -
Martin Kletzander