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

Re: [libvirt] [PATCH v2] virsh: add [--domain DOMAIN] option to domxml-to-native DOMAIN COMMAND



On Wed, May 31, 2017 at 11:27:42AM -0400, Dan wrote:
On Tue, May 30, 2017 at 08:45:37AM +0200, Peter Krempa wrote:
On Mon, May 29, 2017 at 14:08:53 -0400, Daniel Liu wrote:
> 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  | virsh domxml-to-native /dev/stdin'.
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=835476
> ---
>  tools/virsh-domain.c | 52 ++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 40 insertions(+), 12 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index ccb514ef9..929f9c896 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c

[...]

> @@ -9859,30 +9863,54 @@ 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)
> +        vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0 ||
> +        vshCommandOptStringReq(ctl, cmd, "domain", &domain) < 0)

You don't need this variable after the check below, so it's pointless to
extract it.

Right, I finally realized it this morning and deleted this variable.
>          return false;
>
> -    if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0)
> +    VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml);

This macro is documented to work only on booleans, please don't use it
on pointers.

I switched to using VSH_EXCLUSIVE_OPTIONS(string1, string2) in patch v3;
> +
> +    if (domain) {
> +        if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> +            return false;
> +    }
> +
> +    if (dom) {
> +        xmlData = virDomainGetXMLDesc(dom, flags);
> +    } else if (xml) {
> +        if (virFileReadAll(xml, VSH_MAX_XML_FILE, &xmlData) < 0)
> +            goto cleanup;
> +    }
> +
> +    if (!xmlData) {
> +        vshError(ctl, "%s",
> +                 _("need either domain (ID, UUID, or name) or domain XML configuration file path"));

This line is very long.

>          return false;

You'll leak 'dom' here if it was looked up, but getting of the XML
failed.

Also the message is kind of wrong in that case, since you've provided a
valid name, but the XML could not be requested

Thanks a lot for the review; I was being careless.
> +    }
>
> -    configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags);
> -    if (configData != NULL) {
> -        vshPrint(ctl, "%s", configData);
> -        VIR_FREE(configData);
> +    if (!(configData = virConnectDomainXMLToNative(priv->conn, format, xmlData, flags))) {
> +        vshError(ctl, "%s",
> +                 _("convert from domain XML to native command failed"));

For 'xen' the output is not really a command, so this message also is
not very good.

I have never used/tested xen. And actually I do not know where to start
setting it up except thinking about googling. Could you elaborate a bit
more?


For QEMU, the native interface is a command.  For Xen it is not a
command (I think it's some kind of a configuration or some other
definition).  Basically not all formats have commands as output, that's
it, nothing complicated.

There was no error before and it's still fine, so you could've just
skipped the error entirely, but that doesn't really matter.

Attachment: signature.asc
Description: Digital signature


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