Re: [Xen-devel] [PATCH 11/11] add vtpm support to libxl

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Thu, Sep 27, 2012 at 6:11 PM, Matthew Fioravante
<matthew.fioravante@xxxxxxxxxx> wrote:
> This patch adds vtpm support to libxl. It adds vtpm parsing to config
> files and 3 new xl commands:
> vtpm-attach
> vtpm-detach
> vtpm-list
>
> Signed-off-by: Matthew Fioravante <matthew.fioravante@xxxxxxxxxx>

Overall looks good to me -- just a few comments below about the config
file handling (see below).

Thanks for all your work on this.

> @@ -601,6 +605,8 @@ static void domcreate_bootloader_done(libxl__egc *egc,
>  static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *aodevs,
>                                  int ret);
>
> +static void domcreate_attach_vtpms(libxl__egc *egc, libxl__multidev 
> *multidev,
> +                                   int ret);
>  static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *aodevs,
>                                   int ret);
>
> @@ -1084,13 +1090,13 @@ static void domcreate_devmodel_started(libxl__egc 
> *egc,
>      if (d_config->num_nics > 0) {
>          /* Attach nics */
>          libxl__multidev_begin(ao, &dcs->multidev);
> -        dcs->multidev.callback = domcreate_attach_pci;
> +        dcs->multidev.callback = domcreate_attach_vtpms;

Wow -- what a weird convention you've had to try to figure out and
modify here.  Well done. :-)

>          libxl__add_nics(egc, ao, domid, d_config, &dcs->multidev);
>          libxl__multidev_prepared(egc, &dcs->multidev, 0);
>          return;
>      }
>
> -    domcreate_attach_pci(egc, &dcs->multidev, 0);
> +    domcreate_attach_vtpms(egc, &dcs->multidev, 0);
>      return;
>
>  error_out:
> @@ -1098,6 +1104,36 @@ error_out:
>      domcreate_complete(egc, dcs, ret);
>  }
>
> +static void domcreate_attach_vtpms(libxl__egc *egc, libxl__multidev 
> *multidev, int ret) {
> +   libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
> +   STATE_AO_GC(dcs->ao);
> +   int domid = dcs->guest_domid;
> +
> +   libxl_domain_config* const d_config = dcs->guest_config;
> +
> +   if(ret) {
> +      LOG(ERROR, "unable to add nic devices");
> +      goto error_out;
> +   }
> +
> +    /* Plug vtpm devices */
> +    if (d_config->num_vtpms > 0) {
> +        /* Attach vtpms */
> +        libxl__multidev_begin(ao, &dcs->multidev);
> +        dcs->multidev.callback = domcreate_attach_pci;
> +        libxl__add_vtpms(egc, ao, domid, d_config, &dcs->multidev);
> +        libxl__multidev_prepared(egc, &dcs->multidev, 0);
> +        return;
> +    }
> +
> +   domcreate_attach_pci(egc, multidev, 0);
> +   return;
> +
> +error_out:
> +   assert(ret);
> +   domcreate_complete(egc, dcs, ret);
> +}
> +
>  static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *multidev,
>                                   int ret)
>  {
> @@ -1111,7 +1147,7 @@ static void domcreate_attach_pci(libxl__egc *egc, 
> libxl__multidev *multidev,
>      libxl_domain_config *const d_config = dcs->guest_config;
>
>      if (ret) {
> -        LOG(ERROR, "unable to add nic devices");
> +        LOG(ERROR, "unable to add vtpm devices");
>          goto error_out;
>      }
>
[snip]
> @@ -1045,6 +1045,47 @@ static void parse_config_data(const char 
> *config_source,
>          }
>      }
>
> +    if (!xlu_cfg_get_list(config, "vtpm", &vtpms, 0, 0)) {
> +        d_config->num_vtpms = 0;
> +        d_config->vtpms = NULL;
> +        while ((buf = xlu_cfg_get_listitem (vtpms, d_config->num_vtpms)) != 
> NULL) {
> +            libxl_device_vtpm *vtpm;
> +            char * buf2 = strdup(buf);
> +            char *p, *p2;
> +
> +            d_config->vtpms = (libxl_device_vtpm *) realloc(d_config->vtpms, 
> sizeof(libxl_device_vtpm) * (d_config->num_vtpms+1));
> +            vtpm = d_config->vtpms + d_config->num_vtpms;
> +            libxl_device_vtpm_init(vtpm);
> +            vtpm->devid = d_config->num_vtpms;
> +
> +            p = strtok(buf2, ",");
> +            if (!p)
> +                goto skip_vtpm;

Is the purpose of this so that you can have "empty" vtpm slots?
(Since even when skipping, you still increment the num_vtpms counter?)

> +            do {
> +                while(*p == ' ')
> +                    ++p;
> +                if ((p2 = strchr(p, '=')) == NULL)
> +                    break;
> +                *p2 = '\0';
> +                if (!strcmp(p, "backend")) {
> +                    if(domain_qualifier_to_domid(p2 + 1, 
> &(vtpm->backend_domid), 0))
> +                    {
> +                        fprintf(stderr, "Specified backend domain does not 
> exist, defaulting to Dom0\n");
> +                        vtpm->backend_domid = 0;

So wait; if someone specifies a domain, and that turns out not to
work, you just change it to 0?  It seems like if the *specified*
domain doesn't exist, the command should fail instead of choosing
something at random.

> +                    }
> +                } else if(!strcmp(p, "uuid")) {
> +                    if( libxl_uuid_from_string(&vtpm->uuid, p2 + 1) ) {
> +                        fprintf(stderr, "Failed to parse vtpm UUID: %s\n", 
> p2 + 1);
> +                        exit(1);
> +                    }
> +                }

If I'm parsing this right, it looks like you will just silently ignore
other arguments -- it seems like throwing an error would be better;
especially to catch things like typos.  Otherwise, if someone does
something like "uid=[whatever]", it will act like uuid isn't there and
create a new one, instead of alerting the user to the fact that he'd
made a typo in the config file.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel