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

[Martin Kletzander]

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.
-------------- 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/20170531/b0c5d260/attachment-0001.sig>