It'd be good if you could split this patch in two parts - one doing the monitor command stuff, and one doing the XML extensions - since they're functionally independent of each other.
On Tue, Apr 13, 2010 at 02:36:49PM -0400, Chris Lalancette wrote: > @@ -4593,6 +4596,31 @@ int qemudBuildCommandLine(virConnectPtr conn, > ADD_ARG_LIT(current_snapshot->def->name); > } > > + if (def->namespaceData) { > + qemuDomainAdvancedDefPtr adv; > + > + adv = def->namespaceData; > + if (adv->cmdline_extra) { > + const char **progenv = NULL; > + const char **progargv = NULL; > + > + if (qemuStringToArgvEnv(adv->cmdline_extra, &progenv, &progargv) > < 0) > + goto error; > + > + for (i = 0 ; progargv && progargv[i] ; i++) { > + ADD_ARG_LIT(progargv[i]); > + VIR_FREE(progargv[i]); > + } > + VIR_FREE(progargv); > + > + for (i = 0 ; progenv && progenv[i] ; i++) { > + ADD_ENV_LIT(progenv[i]); > + VIR_FREE(progenv[i]); > + } > + VIR_FREE(progenv); > + } > + } > + > ADD_ARG(NULL); > ADD_ENV(NULL); > > diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h > index e0666cb..eef88c4 100644 > --- a/src/qemu/qemu_conf.h > +++ b/src/qemu/qemu_conf.h > @@ -158,6 +158,12 @@ struct qemud_driver { > typedef struct _qemuDomainPCIAddressSet qemuDomainPCIAddressSet; > typedef qemuDomainPCIAddressSet *qemuDomainPCIAddressSetPtr; > > +typedef struct _qemuDomainAdvancedDef qemuDomainAdvancedDef; > +typedef qemuDomainAdvancedDef *qemuDomainAdvancedDefPtr; > +struct _qemuDomainAdvancedDef { > + char *cmdline_extra; > +}; > + This should be storing individual command line args, eg unsigned int ncmdline_extra; char **cmdline_extra; > +static void *qemuDomainDefNamespaceParse(xmlDocPtr xml, > + xmlNodePtr root, > + xmlXPathContextPtr ctxt) > +{ > + qemuDomainAdvancedDefPtr adv = NULL; > + xmlNsPtr ns; > + > + ns = xmlSearchNs(xml, root, BAD_CAST "qemu"); > + if (!ns) > + /* this is fine; it just means there was no qemu namespace listed */ > + return NULL; > + > + if (STRNEQ((const char *)ns->href, QEMU_NAMESPACE_HREF)) { > + qemuReportError(VIR_ERR_INTERNAL_ERROR, > + _("Found namespace '%s' doesn't match expected > '%s'"), > + ns->href, QEMU_NAMESPACE_HREF); > + return NULL; > + } > + > + if (VIR_ALLOC(adv) < 0) { > + virReportOOMError(); > + return NULL; > + } > + > + xmlXPathRegisterNs(ctxt, ns->prefix, ns->href); > + > + adv->cmdline_extra = > virXPathString("string(./qemu:advanced/qemu:commandline/qemu:extra[1])", > + ctxt); > + > + return adv; > +} > +static int qemuDomainDefNamespaceFormatXML(virBufferPtr buf, > + void *nsdata) > +{ > + qemuDomainAdvancedDefPtr adv = nsdata; > + > + virBufferAddLit(buf, " <qemu:advanced>\n"); > + if (adv->cmdline_extra) { > + virBufferAddLit(buf, " <qemu:commandline>\n"); > + virBufferVSprintf(buf, " <qemu:extra>%s</qemu:extra>\n", > + adv->cmdline_extra); > + virBufferAddLit(buf, " </qemu:commandline>\n"); > + } > + virBufferAddLit(buf, " </qemu:advanced>\n"); > + > + return 0; > +} This really does need to represent each argument + environment variable separately. Using an opaque string exposes apps + libvirt to unneccessarily high risk of exploitation with bad / malicious user data, causing one arg to suddenly be parsed as two, or worse. eg, the app may think they are adding -drive file=/some/path but get tricked into using a path '/foo/bar -device pci-host:pciaddr=0x54' which results in the XML <qemu:extra>-drive file=/foo/bar -device pci-host:pciaddr=0x54</qemu:extra> By requiring explicit per arg XML we prevent any possibility of that particular exploit. <qemu:arg>-drive</qemu:arg> <qemu:arg>file=/foo/bar -device pci-host:pciaddr=0x54</qemu:arg> Or <qemu:arg value="-drive" /> <qemu:arg value="file=/foo/bar -device pci-host:pciaddr=0x54" /> This kind of risk is why we use 'virExec' with explicit argv instead of 'system()' or equivalent. Similarly environment variables need to be split too <qemu:env name='FOO' value='bar' /> Yes, this means there is slightly more typing involved, but it is not very much more and cancelled out by the security benefits this brings Finally the <qemu:advanced> wrapper is IMHO redundant information, not least because the concept of 'advanced' is in the eye of the beholder. > + > +static const char *qemuDomainDefNamespaceHref(void) > +{ > + return "xmlns:qemu='" QEMU_NAMESPACE_HREF "'"; > +} > + > +static int qemuDomainLoadAllConfigs(virCapsPtr caps, > + virDomainObjListPtr doms, > + const char *configDir, > + const char *autostartDir, > + int liveStatus, > + virDomainLoadConfigNotify notify, > + void *opaque) > +{ > + struct xmlNamespace ns; > + > + ns.parse = qemuDomainDefNamespaceParse; > + ns.free = qemuDomainDefNamespaceFree; > + ns.format = qemuDomainDefNamespaceFormatXML; > + ns.href = qemuDomainDefNamespaceHref; > + > + return virDomainLoadAllConfigs(caps, doms, configDir, autostartDir, > + liveStatus, notify, &ns, opaque); > +} > + > +static virDomainDefPtr qemuDomainDefParseString(virCapsPtr caps, > + const char *xml, int flags) > +{ > + struct xmlNamespace ns; > + > + ns.parse = qemuDomainDefNamespaceParse; > + ns.free = qemuDomainDefNamespaceFree; > + ns.format = qemuDomainDefNamespaceFormatXML; > + ns.href = qemuDomainDefNamespaceHref; > + > + return virDomainDefParseString(caps, xml, &ns, flags); > +} For the driver-specific state data, I stored the extra parser hooks in the virCapsPtr object struct _virCaps { virCapsHost host; int nguests; virCapsGuestPtr *guests; unsigned char macPrefix[VIR_MAC_PREFIX_BUFLEN]; unsigned int emulatorRequired : 1; void *(*privateDataAllocFunc)(void); void (*privateDataFreeFunc)(void *); int (*privateDataXMLFormat)(virBufferPtr, void *); int (*privateDataXMLParse)(xmlXPathContextPtr, void *); }; Admittedly this is a bit of a hack, but I think we might as well keep going with it and put the xmlNamespace struct there too. This avoids the need to create wrappers around the XML functions from domain_conf.h Could perhaps define a struct virDomainParser { void *(*privateDataAllocFunc)(void); void (*privateDataFreeFunc)(void *); int (*privateDataXMLFormat)(virBufferPtr, void *); int (*privateDataXMLParse)(xmlXPathContextPtr, void *); struct xmlNamespace nsInfo; } And just reference that in virCaps to keep it cleaner. > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c > index 2904201..92f9dd0 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -2222,3 +2222,49 @@ int qemuMonitorJSONDeleteSnapshot(qemuMonitorPtr mon, > const char *name) > virJSONValueFree(reply); > return ret; > } > + > +int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon, > + const char *cmd, > + char **reply) > +{ > + int ret = -1; > + qemuMonitorMessage msg; > + > + *reply = NULL; > + > + memset(&msg, 0, sizeof msg); > + > + if (virAsprintf(&msg.txBuffer, "%s\r\n", cmd) < 0) { > + virReportOOMError(); > + goto cleanup; > + } > + msg.txLength = strlen(msg.txBuffer); > + msg.txFD = -1; > + > + ret = qemuMonitorSend(mon, &msg); > + > + /* If we got ret==0, but not reply data something rather bad > + * went wrong, so lets fake an EIO error */ > + if (!msg.rxBuffer && ret == 0) { > + msg.lastErrno = EIO; > + ret = -1; > + } > + > + if (ret == 0) { > + *reply = strdup(msg.rxBuffer); > + if (*reply == NULL) { > + ret = -1; > + virReportOOMError(); > + goto cleanup; > + } > + } > + else > + virReportSystemError(msg.lastErrno, > + _("cannot send monitor command '%s'"), cmd); > + > +cleanup: > + VIR_FREE(msg.txBuffer); > + VIR_FREE(msg.rxBuffer); > + > + return ret; > +} This duplicates rather alot of the qemuMonitorJSONCommandWithFd() code. I think it'd be preferable to instead parse/serialize it to JSON and call the normal qemuMonitorJSONCommand() method, even though that is technically adding a redundant parse step. virJSONValuePtr virJSONValueFromString(const char *jsonstring); char *virJSONValueToString(virJSONValuePtr object); should do the trick fairly nicely. > diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c > index 9942768..65760f5 100644 > --- a/src/qemu/qemu_monitor_text.c > +++ b/src/qemu/qemu_monitor_text.c > @@ -2438,3 +2438,14 @@ cleanup: > VIR_FREE(reply); > return ret; > } > + > +int qemuMonitorTextArbitraryCommand(qemuMonitorPtr mon, const char *cmd, > char **reply) > +{ > + if (qemuMonitorCommand(mon, cmd, reply)) { > + qemuReportError(VIR_ERR_OPERATION_FAILED, > + _("failed to run cmd '%s'"), cmd); > + return -1; > + } > + > + return 0; > +} This misses the trailing '\r\n' sequence on the command. It is also worth validating that the command doesn't include a \r\n sequence here, otherwise libvirt's response handling will become incredibly confused. Since we can now represent extra command line args, the qemuParseCommandLine method itself can be enhanced, so that if it finds args it does not yet understand, it just adds an extra "<qemu:arg>" param for each one it gets. That would make the output of the native-to-xml function almost 100% compatible with the original QEMU args the user supplied, or at least more reliably roundtrip-able. A couple of test data files for the XML extension is desirable too. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list