[libvirt] [PATCH] virsh: add --domain option for domain-to-native
Martin Kletzander
mkletzan at redhat.com
Mon May 29 18:53:18 UTC 2017
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".
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170529/72746c49/attachment-0001.sig>
More information about the libvir-list
mailing list