[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] virsh: add --domain option for domain-to-native



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

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]