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@xxxxxxxxxxxxxxxxxx> > Reviewed-by: Corey Bryant <coreyb@xxxxxxxxxxxxxxxxxx> > Tested-by: Corey Bryant <coreyb@xxxxxxxxxxxxxxxxxx> > > --- > 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@xxxxxxxxxxxxxxxxxx> > + */ > + > +#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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list