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

On Mon, May 29, 2017 at 02:08:53PM -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(-) > Just few things I forgot to sent, so they remained in my drafts folder. > 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 have to do this, you don't use the domain name anywhere, just the information whether it's present or not. > return false; > > - if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &xmlData) < 0) > + VSH_EXCLUSIVE_OPTIONS_VAR(domain, xml); > + > + if (domain) { > + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) > + return false; And instead of this, you can just do: if (vshCommandOptBool("domain") && (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))) return false;

> + } > + > + if (dom) { > + xmlData = virDomainGetXMLDesc(dom, flags); xmlData can be NULL after this in case of an error. > + } else if (xml) { > + if (virFileReadAll(xml, VSH_MAX_XML_FILE, &xmlData) < 0) > + goto cleanup; > + } > + > + if (!xmlData) { So this could just be else {} of the last condition and if xmlData is NULL after calling virDomainGetXMLDesc(), there should be different error message. Other things were pointed out already, I guess.