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".