[libvirt] [PATCH v4 04/11] Helper functions for host TPM support

[Daniel P. Berrange]

On Fri, Apr 05, 2013 at 10:05:55AM -0400, Stefan Berger wrote:
> Implement helper functions to find the TPM's sysfs cancel file.
> 
> Signed-off-by: Stefan Berger <stefanb at linux.vnet.ibm.com>
> Reviewed-by: Corey Bryant <coreyb at linux.vnet.ibm.com>
> Tested-by: Corey Bryant <coreyb at linux.vnet.ibm.com>
> 
> ---
>  po/POTFILES.in           |    1 
>  src/Makefile.am          |    1 
>  src/libvirt_private.syms |    4 +
>  src/util/virtpm.c        |  111 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/virtpm.h        |   27 +++++++++++
>  5 files changed, 144 insertions(+)
> 
> Index: libvirt/src/Makefile.am
> ===================================================================
> --- libvirt.orig/src/Makefile.am
> +++ libvirt/src/Makefile.am
> @@ -122,6 +122,7 @@ UTIL_SOURCES =							\
>  		util/virthreadwin32.h				\
>  		util/virthreadpool.c util/virthreadpool.h	\
>  		util/virtime.h util/virtime.c			\
> +		util/virtpm.h util/virtpm.c			\
>  		util/virtypedparam.c util/virtypedparam.h	\
>  		util/virusb.c util/virusb.h			\
>  		util/viruri.h util/viruri.c			\
> Index: libvirt/src/util/virtpm.c
> ===================================================================
> --- /dev/null
> +++ libvirt/src/util/virtpm.c
> @@ -0,0 +1,111 @@
> +/*
> + * virtpm.c: TPM support
> + *
> + * Copyright (C) 2013 IBM Corporation
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + * Author: Stefan Berger <stefanb at linux.vnet.ibm.com>
> + */
> +
> +#include <config.h>
> +
> +#include <stdio.h>
> +#include <dirent.h>
> +#include <unistd.h>
> +#include <sys/stat.h>
> +
> +#include "virobject.h"
> +#include "viralloc.h"
> +#include "virutil.h"
> +#include "virerror.h"
> +#include "virbuffer.h"
> +#include "virtpm.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_NONE
> +
> +/*
> + * Check whether the given base path, e.g.,  /sys/class/misc/tpm0/device,
> + * is the sysfs entry of a TPM. A TPM sysfs entry should be uniquely
> + * recognizable by the file entries 'pcrs' and 'cancel'.
> + * Upon success the basepath with '/cancel' appended is returned, NULL
> + * otherwise.
> + */
> +static char *
> +virTPMCheckSysfsCancel(char *basepath)
> +{
> +    char *path = NULL;
> +    struct stat statbuf;
> +
> +    if (virAsprintf(&path, "%s/pcrs", basepath) < 0) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +    if (stat(path, &statbuf) == -1 || !S_ISREG(statbuf.st_mode))
> +        goto cleanup;

This jumps to an error exit path, but without calling virReportError

> +
> +    VIR_FREE(path);
> +
> +    if (virAsprintf(&path, "%s/cancel", basepath) < 0) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    if (stat(path, &statbuf) == -1 || !S_ISREG(statbuf.st_mode))
> +        goto cleanup;

So does this.

> +
> +    return path;
> +
> +cleanup:
> +    VIR_FREE(path);
> +    return NULL;
> +}

All error paths must report errors.

> +
> +
> +char *
> +virTPMFindCancelPath(void)
> +{
> +    unsigned int idx;
> +    int len;
> +    DIR *pnp_dir;
> +    char *path = NULL, *basepath = NULL;
> +    struct dirent entry, *result;
> +
> +    pnp_dir = opendir("/sys/class/misc");
> +    if (pnp_dir != NULL) {

You should report an error if this fails, not silently ignore it.


> +        while (readdir_r(pnp_dir, &entry, &result) == 0 &&
> +               result != NULL) {

readdir_r is not required. Standard  readdir() is threadsafe
when you only have 1 thread using the "DIR *" pointer.

> +            if (sscanf(entry.d_name, "tpm%u%n", &idx, &len) < 1 ||
> +                len <= strlen("tpm") ||
> +                len != strlen(entry.d_name)) {
> +                continue;
> +            }

As a general rule we prefer to avoid use of scanf & friends.

I'd do something more like

   if (!STRPREFIX(entry.d_name, "tpm"))
      continue;

   if (virStrToLong_i(entry.d_name + strlen("tpm"), NULL, &idx) < 0)
       ...report parsing error... 

> +            if (virAsprintf(&basepath, "/sys/class/misc/%s/device",
> +                            entry.d_name) < 0) {
> +                virReportOOMError();
> +                break;
> +            }
> +            if ((path = virTPMCheckSysfsCancel(basepath)))
> +                break;

The virTPMCheckSysfsCancel() method will raise an error, but you
are silently ignoring errors here. Either clear the error with
virResetLastError() if you really need to ignore it, or propagate
the error to the caller

> +
> +            VIR_FREE(basepath);
> +        }

When leaving the loop should also report an error if
readdir() return an error code, rather than EOF.

> +        closedir(pnp_dir);
> +    }
> +
> +    VIR_FREE(basepath);
> +
> +    return path;
> +}

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|