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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list