12:25 p.m.
On 01/14/2011 05:41 AM, Daniel P. Berrange wrote:
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 ?
Submitted, but not yet included (so another reason that I shouldn't
necessarily push this upstream yet).
http://patchwork.ozlabs.org/patch/78297/
Also there is where the lack of security driver integration
will cause problems, because QEMU won't start.
Yes, will address that in v3.
> @@ -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
My understanding is that we only need one -device usb-ccid; that one
device is necessary to provide the bus shared by all other -device
ccid-card-* entries. So the alias defined above is for the ccid-card.
I guess that means you want a hard-coded alias here:
virCommandAddArgList(cmd, "-device", "usb-ccid,id=ccid0", NULL);
But with Cole's recent sound patch, we added -device hda-output without
any id= notation (another case where in addition to the real -device
intel-hda,id=sound4 to match the <sound> element, we also needed the
secondary bus -device hda-sound as behind-the-scenes glue).
> + }
> + 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:
I assumed that the alias was only needed for the case of tying a chardev
to a -device ccid-card-passthru (the -device ccid-card-emulated does not
have an associated chardev). Which is why the DEVICE_TYPE_PASSTHROUGH
block _does_ use the alias (just the TYPE_HOST and
TYPE_HOST_CERTIFICATES did not use it, but it wasn't generated for those
cases).
> + 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.
It's only confusing if the filename contains a literal comma, so v3 will
reject filenames with a literal comma.
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.
Nah - a literal comma is rare enough to not go with this complexity
unless someone has a compelling reason why we should.
> + 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.
But we don't do that for any of our other -chardevs. Oh, are you saying
that we first need to patch things to carry dual aliases to distinguish
between -chardev and its associated -device, and that every object
passed to qemu (and not just -chardev) should be given an alias?
Is that a prerequisite to getting this series in, or should it be a
separate cleanup patch that can come later?
> + 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.
That's how Alon implemented it. -device usb-ccid is instantiated
exactly once, and is not tied to any -chardev. -device
ccid-card-emulated automatically uses the services of -device usb-ccid,
and does not need any -chardev. And -device ccid-card-passthru
automatically uses the services of -device usb-ccid, and requires an
associated -chardev (Alon has only tested with a tcp chardev and with a
spicevmc,name=smartcard chardev). At this point, your complaints seem
to be more about whether Alon's qemu command line implementation makes
sense (which, given that the smartcard patches have been proposed but
are not yet upstream, may mean that the final qemu implementation will
change and my libvirt patch would have to equally change to match),
rather than whether I'm correctly mapping to the command line that Alon
documented:
http://cgit.freedesktop.org/~alon/qemu/commit/?h=usb_ccid.v15&id=024a...
Of course, providing some .args test files in v3 will help make it a
little easier to review.
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.
That I'm not sure about. Alon said that the guest sees the smartcard as
a USB device, but didn't mention anything about whether -device
ccid-card-* can take additional USB addressing parameters to lock down
where the device appears on the guest's USB bus.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org