On 05/03/2013 02:07 PM, Osier Yang wrote: > From: Han Cheng <hanc.fnst@xxxxxxxxxxxxxx> > > This patch adds util functions for scsi hostdev. > > Signed-off-by: Han Cheng <hanc.fnst@xxxxxxxxxxxxxx> > Signed-off-by: Osier Yang <jyang@xxxxxxxxxx> > > --- > v3 - v4: > * Use strdup instead of virAsprintf if the format string is "%s" > > v2.5 - v3: > * A couple "char *" to "const char *" > * s/unsigned int readonly/bool readonly/ > * Use STRPREIFX instead of the wrong use of STRSKIP (which can introduce bug) > for virSCSIDeviceGetAdapterId. > * Improve virSCSIDeviceNew > > I'm not fan of introducing another list like virPCIDeviceList, which > brought much trouble for us, but it doesn't hurt much to do clean up together > later, since they are very similar. > --- > po/POTFILES.in | 1 + > src/Makefile.am | 1 + > src/libvirt_private.syms | 22 +++ > src/util/virscsi.c | 408 +++++++++++++++++++++++++++++++++++++++++++++++ > src/util/virscsi.h | 84 ++++++++++ > 5 files changed, 516 insertions(+) > create mode 100644 src/util/virscsi.c > create mode 100644 src/util/virscsi.h > I'll start by noting, I'm still not fully aware of all the "norms" regarding all that needs to be touched when adding a new file and all that needs to be done when adding new API's - so I could miss something that perhaps someone else will pick up on... > diff --git a/po/POTFILES.in b/po/POTFILES.in > index bf5a864..f3ea4da 100644 > --- a/po/POTFILES.in > +++ b/po/POTFILES.in > @@ -174,6 +174,7 @@ src/util/virportallocator.c > src/util/virprocess.c > src/util/virrandom.c > src/util/virsexpr.c > +src/util/virscsi.c > src/util/virsocketaddr.c > src/util/virstatslinux.c > src/util/virstoragefile.c > diff --git a/src/Makefile.am b/src/Makefile.am > index 299b8fd..e71975a 100644 > --- a/src/Makefile.am > +++ b/src/Makefile.am > @@ -111,6 +111,7 @@ UTIL_SOURCES = \ > util/virportallocator.c util/virportallocator.h \ > util/virprocess.c util/virprocess.h \ > util/virrandom.h util/virrandom.c \ > + util/virscsi.c util/virscsi.h \ > util/virsexpr.c util/virsexpr.h \ > util/virsocketaddr.h util/virsocketaddr.c \ > util/virstatslinux.c util/virstatslinux.h \ > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 98660dc..64e2816 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1687,6 +1687,28 @@ virRandomGenerateWWN; > virRandomInt; > > > +# util/virscsi.h > +virSCSIDeviceFileIterate; > +virSCSIDeviceFree; > +virSCSIDeviceGetAdapter; > +virSCSIDeviceGetBus; > +virSCSIDeviceGetDevStr; > +virSCSIDeviceGetName; > +virSCSIDeviceGetReadonly; > +virSCSIDeviceGetTarget; > +virSCSIDeviceGetUnit; > +virSCSIDeviceGetUsedBy; > +virSCSIDeviceListAdd; > +virSCSIDeviceListCount; > +virSCSIDeviceListDel; > +virSCSIDeviceListFind; > +virSCSIDeviceListGet; > +virSCSIDeviceListNew; > +virSCSIDeviceListSteal; > +virSCSIDeviceNew; > +virSCSIDeviceSetUsedBy; > + > + > # util/virsexpr.h > sexpr2string; > sexpr_append; > diff --git a/src/util/virscsi.c b/src/util/virscsi.c > new file mode 100644 > index 0000000..2d6bd8c > --- /dev/null > +++ b/src/util/virscsi.c > @@ -0,0 +1,408 @@ > +/* > + * virscsi.c: helper APIs for managing host SCSI devices > + * > + * Copyright (C) 2013 Fujitsu, Inc. > + * > + * 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/>. > + * > + * Authors: > + * Han Cheng <hanc.fnst@xxxxxxxxxxxxxx> > + */ > + > +#include <config.h> > + > +#include <dirent.h> > +#include <fcntl.h> > +#include <inttypes.h> > +#include <limits.h> > +#include <stdio.h> > +#include <string.h> > +#include <sys/types.h> > +#include <sys/stat.h> > +#include <unistd.h> > + > +#include "virscsi.h" > +#include "virlog.h" > +#include "viralloc.h" > +#include "virutil.h" > +#include "virstring.h" > +#include "virerror.h" > + > +#define SYSFS_SCSI_DEVICES "/sys/bus/scsi/devices" > + > +/* For virReportOOMError() and virReportSystemError() */ > +#define VIR_FROM_THIS VIR_FROM_NONE > + > +struct _virSCSIDevice { > + unsigned int adapter; > + unsigned int bus; > + unsigned int target; > + unsigned int unit; > + > + char *name; /* adapter:bus:target:unit */ > + char *id; /* model:vendor */ > + char *path; > + const char *used_by; /* name of the domain using this dev */ > + > + bool readonly; > +}; > + > +struct _virSCSIDeviceList { > + virObjectLockable parent; > + unsigned int count; > + virSCSIDevicePtr *devs; > +}; > + > +static virClassPtr virSCSIDeviceListClass; > + > +static void virSCSIDeviceListDispose(void *obj); > + > +static int > +virSCSIOnceInit(void) > +{ > + if (!(virSCSIDeviceListClass = virClassNew(virClassForObjectLockable(), > + "virSCSIDeviceList", > + sizeof(virSCSIDeviceList), > + virSCSIDeviceListDispose))) > + return -1; > + > + return 0; > +} > + > +VIR_ONCE_GLOBAL_INIT(virSCSI) > + > +static int > +virSCSIDeviceGetAdapterId(const char *adapter, > + unsigned int *adapter_id) > +{ > + if (STRPREFIX(adapter, "scsi_host")) { > + if (virStrToLong_ui(adapter + strlen("scsi_host"), > + NULL, 0, adapter_id) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Cannot parse adapter '%s'"), adapter); > + return -1; > + } > + } > + hmm... which makes me think, does the description in patch 1/25 need to be adjusted to indicate that "scsi_host" is expected as part of the adapter name? and that adapters are thus identified by the numeric value after the prefix? > + return 0; > +} > + > +char * > +virSCSIDeviceGetDevStr(const char *adapter, > + unsigned int bus, > + unsigned int target, > + unsigned int unit) > +{ > + DIR *dir = NULL; > + struct dirent *entry; > + char *path = NULL; > + char *sg = NULL; > + unsigned int adapter_id; > + > + if (virSCSIDeviceGetAdapterId(adapter, &adapter_id) < 0) > + return NULL; > + > + if (virAsprintf(&path, > + SYSFS_SCSI_DEVICES "/%d:%d:%d:%d/scsi_generic", > + adapter_id, bus, target, unit) < 0) { > + virReportOOMError(); > + return NULL; > + } > + > + if (!(dir = opendir(path))) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Failed to open %s"), path); > + goto cleanup; > + } > + > + while ((entry = readdir(dir))) { > + if (entry->d_name[0] == '.') > + continue; > + > + if (!(sg = strdup(entry->d_name))) { > + virReportOOMError(); > + goto cleanup; > + } > + } > + > +cleanup: > + closedir(dir); > + VIR_FREE(path); > + return sg; > +} > + > +virSCSIDevicePtr > +virSCSIDeviceNew(const char *adapter, > + unsigned int bus, > + unsigned int target, > + unsigned int unit, > + bool readonly) > +{ > + virSCSIDevicePtr dev, ret = NULL; > + char *sg = NULL; > + char *vendor_path = NULL; > + char *model_path = NULL; > + char *vendor = NULL; > + char *model = NULL; > + > + if (VIR_ALLOC(dev) < 0) { > + virReportOOMError(); > + return NULL; > + } > + > + dev->bus = bus; > + dev->target = target; > + dev->unit = unit; > + dev->readonly = readonly; > + > + if (!(sg = virSCSIDeviceGetDevStr(adapter, bus, target, unit))) > + goto cleanup; 'sg' is now strdup()'d memory > + > + if (virSCSIDeviceGetAdapterId(adapter, &dev->adapter) < 0) > + goto cleanup; > + > + if (virAsprintf(&dev->name, "%d:%d:%d:%d", dev->adapter, > + dev->bus, dev->bus, dev->unit) < 0 || > + virAsprintf(&dev->path, "/dev/%s", sg) < 0) { > + virReportOOMError(); > + goto cleanup; > + } > + > + if (access(dev->path, F_OK) != 0) { > + virReportSystemError(errno, > + _("Device %s not found: could not access %s"), > + dev->name, dev->path); Is it really 'not found'? Perhaps "SCSI device '%s': could not access '%s'"? > + goto cleanup; > + } > + > + if (virAsprintf(&vendor_path, > + SYSFS_SCSI_DEVICES "/%s/vendor", dev->name) < 0 || > + virAsprintf(&model_path, > + SYSFS_SCSI_DEVICES "/%s/model", dev->name) < 0) { > + virReportOOMError(); > + goto cleanup; > + } > + > + if (virFileReadAll(vendor_path, 1024, &vendor) < 0) > + goto cleanup; > + > + if (virFileReadAll(model_path, 1024, &model) < 0) > + goto cleanup; > + > + virTrimSpaces(vendor, NULL); > + virTrimSpaces(model, NULL); > + > + if (virAsprintf(&dev->id, "%s:%s", vendor, model) < 0) { > + virReportOOMError(); > + goto cleanup; > + } > + > + ret = dev; > +cleanup: VIR_FREE(sg); > + VIR_FREE(vendor); > + VIR_FREE(model); > + VIR_FREE(vendor_path); > + VIR_FREE(model_path); > + if (!ret) > + virSCSIDeviceFree(dev); > + return ret; > +} > + > +void > +virSCSIDeviceFree(virSCSIDevicePtr dev) > +{ > + if (!dev) > + return; > + > + VIR_FREE(dev->id); > + VIR_FREE(dev->name); > + VIR_FREE(dev->path); > + VIR_FREE(dev); > +} > + > +void > +virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, > + const char *name) > +{ > + dev->used_by = name; > +} > + > +const char * > +virSCSIDeviceGetUsedBy(virSCSIDevicePtr dev) > +{ > + return dev->used_by; > +} > + > +const char * > +virSCSIDeviceGetName(virSCSIDevicePtr dev) > +{ > + return dev->name; > +} > + > +unsigned int > +virSCSIDeviceGetAdapter(virSCSIDevicePtr dev) > +{ > + return dev->adapter; > +} > + > +unsigned int > +virSCSIDeviceGetBus(virSCSIDevicePtr dev) > +{ > + return dev->bus; > +} > + > +unsigned int > +virSCSIDeviceGetTarget(virSCSIDevicePtr dev) > +{ > + return dev->target; > +} > + > +unsigned int > +virSCSIDeviceGetUnit(virSCSIDevicePtr dev) > +{ > + return dev->unit; > +} > + > +bool > +virSCSIDeviceGetReadonly(virSCSIDevicePtr dev) > +{ > + return dev->readonly; > +} > + > +int > +virSCSIDeviceFileIterate(virSCSIDevicePtr dev, > + virSCSIDeviceFileActor actor, > + void *opaque) > +{ > + return (actor)(dev, dev->path, opaque); > +} > + > +virSCSIDeviceListPtr > +virSCSIDeviceListNew(void) > +{ > + virSCSIDeviceListPtr list; > + > + if (virSCSIInitialize() < 0) > + return NULL; > + > + if (!(list = virObjectLockableNew(virSCSIDeviceListClass))) > + return NULL; > + > + return list; > +} > + > +static void > +virSCSIDeviceListDispose(void *obj) > +{ > + virSCSIDeviceListPtr list = obj; > + int i; > + > + for (i = 0; i < list->count; i++) > + virSCSIDeviceFree(list->devs[i]); > + > + VIR_FREE(list->devs); > +} > + > +int > +virSCSIDeviceListAdd(virSCSIDeviceListPtr list, > + virSCSIDevicePtr dev) > +{ > + if (virSCSIDeviceListFind(list, dev)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Device %s already exists"), > + dev->name); > + return -1; > + } > + > + if (VIR_REALLOC_N(list->devs, list->count + 1) < 0) { > + virReportOOMError(); > + return -1; > + } > + > + list->devs[list->count++] = dev; > + > + return 0; > +} > + > +virSCSIDevicePtr > +virSCSIDeviceListGet(virSCSIDeviceListPtr list, int idx) > +{ > + if (idx >= list->count || idx < 0) > + return NULL; > + > + return list->devs[idx]; > +} > + > +int > +virSCSIDeviceListCount(virSCSIDeviceListPtr list) > +{ > + return list->count; > +} > + > +virSCSIDevicePtr > +virSCSIDeviceListSteal(virSCSIDeviceListPtr list, > + virSCSIDevicePtr dev) > +{ > + virSCSIDevicePtr ret = NULL; > + int i; > + > + for (i = 0; i < list->count; i++) { > + if (list->devs[i]->adapter != dev->adapter || > + list->devs[i]->bus != dev->bus || > + list->devs[i]->target != dev->target || > + list->devs[i]->unit != dev->unit) > + continue; > + > + ret = list->devs[i]; > + > + if (i != list->count--) > + memmove(&list->devs[i], > + &list->devs[i+1], > + sizeof(*list->devs) * (list->count - i)); Just checking math and auto decrement order and making sure you get the result you want. It's the for loop end value decrement inside the loop which initially got my attention. If "i=1" and "count=2" prior to the "if", then once we get here the 3rd arg would be 0 (zero), right? It seems you tried to "steal" what virPCIDeviceListSteal() did, but avoided or tried to include the virPCIDeviceListFindIndex() functionality... Perhaps you should make use of the virSCSIDeviceListFind() just like the PCI code. ACK with the issues fixed. John > + > + if (VIR_REALLOC_N(list->devs, list->count) < 0) { > + ; /* not fatal */ > + } > + > + break; > + } > + > + return ret; > +} > + > +void > +virSCSIDeviceListDel(virSCSIDeviceListPtr list, > + virSCSIDevicePtr dev) > +{ > + virSCSIDevicePtr ret = virSCSIDeviceListSteal(list, dev); > + virSCSIDeviceFree(ret); > +} > + > +virSCSIDevicePtr > +virSCSIDeviceListFind(virSCSIDeviceListPtr list, > + virSCSIDevicePtr dev) > +{ > + int i; > + > + for (i = 0; i < list->count; i++) { > + if (list->devs[i]->adapter == dev->adapter && > + list->devs[i]->bus == dev->bus && > + list->devs[i]->target == dev->target && > + list->devs[i]->unit == dev->unit) > + return list->devs[i]; > + } > + > + return NULL; > +} > diff --git a/src/util/virscsi.h b/src/util/virscsi.h > new file mode 100644 > index 0000000..9d63acc > --- /dev/null > +++ b/src/util/virscsi.h > @@ -0,0 +1,84 @@ > +/* > + * virscsi.h: helper APIs for managing host SCSI devices > + * > + * Copyright (C) 2013 Fujitsu, Inc. > + * > + * 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/>. > + * > + * Authors: > + * Han Cheng <hanc.fnst@xxxxxxxxxxxxxx> > + */ > + > +#ifndef __VIR_SCSI_H__ > +# define __VIR_SCSI_H__ > + > +# include "internal.h" > +# include "virobject.h" > + > +typedef struct _virSCSIDevice virSCSIDevice; > +typedef virSCSIDevice *virSCSIDevicePtr; > + > +typedef struct _virSCSIDeviceList virSCSIDeviceList; > +typedef virSCSIDeviceList *virSCSIDeviceListPtr; > + > +char *virSCSIDeviceGetDevStr(const char *adapter, > + unsigned int bus, > + unsigned int target, > + unsigned int unit); > + > +virSCSIDevicePtr virSCSIDeviceNew(const char *adapter, > + unsigned int bus, > + unsigned int target, > + unsigned int unit, > + bool readonly); > + > +void virSCSIDeviceFree(virSCSIDevicePtr dev); > +void virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *name); > +const char *virSCSIDeviceGetUsedBy(virSCSIDevicePtr dev); > +const char *virSCSIDeviceGetName(virSCSIDevicePtr dev); > +unsigned int virSCSIDeviceGetAdapter(virSCSIDevicePtr dev); > +unsigned int virSCSIDeviceGetBus(virSCSIDevicePtr dev); > +unsigned int virSCSIDeviceGetTarget(virSCSIDevicePtr dev); > +unsigned int virSCSIDeviceGetUnit(virSCSIDevicePtr dev); > +bool virSCSIDeviceGetReadonly(virSCSIDevicePtr dev); > + > +/* > + * Callback that will be invoked once for each file > + * associated with / used for SCSI host device access. > + * > + * Should return 0 if successfully processed, or > + * -1 to indicate error and abort iteration > + */ > +typedef int (*virSCSIDeviceFileActor)(virSCSIDevicePtr dev, > + const char *path, void *opaque); > + > +int virSCSIDeviceFileIterate(virSCSIDevicePtr dev, > + virSCSIDeviceFileActor actor, > + void *opaque); > + > +virSCSIDeviceListPtr virSCSIDeviceListNew(void); > +int virSCSIDeviceListAdd(virSCSIDeviceListPtr list, > + virSCSIDevicePtr dev); > +virSCSIDevicePtr virSCSIDeviceListGet(virSCSIDeviceListPtr list, > + int idx); > +int virSCSIDeviceListCount(virSCSIDeviceListPtr list); > +virSCSIDevicePtr virSCSIDeviceListSteal(virSCSIDeviceListPtr list, > + virSCSIDevicePtr dev); > +void virSCSIDeviceListDel(virSCSIDeviceListPtr list, > + virSCSIDevicePtr dev); > +virSCSIDevicePtr virSCSIDeviceListFind(virSCSIDeviceListPtr list, > + virSCSIDevicePtr dev); > + > +#endif /* __VIR_SCSI_H__ */ > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list