On Thu, Jan 13, 2011 at 05:34:37PM -0700, Eric Blake wrote:
> Still to go - add .args files to match .xml files in testsuite
>
> * src/qemu/qemu_command.c (qemuBuildCommandLine): Emit smartcard
> options.
> (qemuAssignDeviceAliases): Assign an alias for smartcard passthrough.
> * tests/qemuxml2argvtest.c (mymain): Three new tests.
> * tests/qemuxml2argvdata/...arg: Three new files.
> ---
>
> Well, as you can see, the tests/ part isn't done yet.
>
> src/qemu/qemu_command.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
What is the status of this support wrt upstream QEMU ? Has the
smartcard stuff been accepted into GIT yet ?
Also there is where the lack of security driver integration
will cause problems, because QEMU won't start.
> 1 files changed, 49 insertions(+), 0 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 205c303..d02241f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -604,6 +604,11 @@ qemuAssignDeviceAliases(virDomainDefPtr def, unsigned long long qemuCmdFlags)
> if (virAsprintf(&def->channels[i]->info.alias, "channel%d", i) < 0)
> goto no_memory;
> }
> + for (i = 0; i < def->nsmartcards ; i++) {
> + if (def->smartcards[i]->type == VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH &&
> + virAsprintf(&def->smartcards[i]->info.alias, "smartcard%d", i) < 0)
> + goto no_memory;
> + }
You've assigned device aliases here....
> if (def->console) {
> if (virAsprintf(&def->console->info.alias, "console%d", i) < 0)
> goto no_memory;
> @@ -3332,6 +3337,50 @@ qemuBuildCommandLine(virConnectPtr conn,
> }
> }
>
> + if (def->nsmartcards) {
> + /* Requires -chardev and -device usb-ccid */
> + if (!(qemuCmdFlags & QEMUD_CMD_FLAG_CHARDEV) ||
> + !(qemuCmdFlags & QEMUD_CMD_FLAG_USB_CCID)) {
> + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("smartcard requires QEMU to support -usb-ccid"));
> + goto error;
> + }
> + virCommandAddArgList(cmd, "-device", "usb-ccid", NULL);
Ideally this would have an id= field too, eg if=ccid0
> + }
> + for (i = 0 ; i < def->nsmartcards ; i++) {
> + virDomainSmartcardDefPtr smartcard = def->smartcards[i];
> + char *devstr;
> + virBuffer smartcard_buf = VIR_BUFFER_INITIALIZER;
> + int j;
> +
....But in this following block of code never added the aliases to
the -device options:
> + switch (smartcard->type) {
> + case VIR_DOMAIN_SMARTCARD_TYPE_HOST:
> + virCommandAddArgList(cmd, "-device", "ccid-card-emulated", NULL);
> + break;
> + case VIR_DOMAIN_SMARTCARD_TYPE_HOST_CERTIFICATES:
> + virCommandAddArg(cmd, "-device");
> + virBufferAddLit(&smartcard_buf,
> + "ccid-card-emulated,backend=certificates");
> + for (j = 0; j < VIR_DOMAIN_SMARTCARD_NUM_CERTIFICATES; j++)
> + virBufferVSprintf(&smartcard_buf, ",cert%d=%s", j + 1,
> + smartcard->data.cert.file[j]);
I guess we need some kind of escaping of special chars in the filename
here since we append 3 in a row, it might confuse QEMU's parser. Not
sure offhand how QEMU expects that to work.
Or since we already depend on existance of -device, we might want to use
'-set' for setting the filenames and thus avoid need to escape, eg
-device ccid-card-emulated,backend=certificates,id=smartcard0
-set ccid-card-emulated.smartcard0.cert0=$FILENAME
-set ccid-card-emulated.smartcard0.cert1=$FILENAME
-set ccid-card-emulated.smartcard0.cert2=$FILENAME
albeit at the cost of much longer command lines.
> + virCommandAddArgBuffer(cmd, &smartcard_buf);
> + break;
> + case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
> + virCommandAddArg(cmd, "-chardev");
> + if (!(devstr = qemuBuildChrChardevStr(&smartcard->data.passthru,
> + smartcard->info.alias)))
> + goto error;
The 'id' setting for the chardev should really have a prefix
on it, because we won't want it to be the same as the 'id'
setting used for the -device option.
> + virCommandAddArg(cmd, devstr);
> + VIR_FREE(devstr);
> +
> + virCommandAddArg(cmd, "-device");
> + virCommandAddArgFormat(cmd, "ccid-card-passthru,chardev=%s",
> + smartcard->info.alias);
> + break;
> + }
> + }
> +
> if (!def->nserials) {
> /* If we have -device, then we set -nodefault already */
> if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE))
We currently only provide a single '-device usb-ccid' device. It looks
like we're relying on the ccid-card-XXX devices automagically associating
themselves with that. It would be better to explicitly link them up
by specifying the 'id' of the 'usb-ccid' device to which they must be
associated. If QEMU doesn't have such an explicit link, it ought to
be added.
Also, does the order of '-device ccid-card-XXX' devices matter ie is
the ordering a guest visible ABI ? If so, then we need to track an
explicit address against each <smartcard> so that we can selectively
hotplug/hotunplug devices, and have a stable ABI across migration.
Regards,
Daniel
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list