[libvirt] [RFC PATCHv2 5/5] WIP: smartcard: turn on qemu support

[Daniel P. Berrange]

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