Re: [libvirt] [PATCH 03/25] utils: util functions for scsi hostdev

On 07/05/13 01:20, John Ferlan wrote: > On 05/03/2013 02:07 PM, Osier Yang wrote: >> From: Han Cheng <hanc.fnst(a)cn.fujitsu.com&gt; >> >> This patch adds util functions for scsi hostdev. >> >> Signed-off-by: Han Cheng <hanc.fnst(a)cn.fujitsu.com&gt; >> Signed-off-by: Osier Yang <jyang(a)redhat.com&gt; >> >> --- >> 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(a)cn.fujitsu.com&gt; >> + */ >> + >> +#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? No, forcely checking it when parsing is not a good idea, just for QEMU/KVM driver, we want it has a prefix "scsi_host", it might be different for other drivers, though currently we only support qemu driver. Even checking it here is not that good, but given that this helper is only used by qemu driver now, I think it's fine, it can be changed if need in future. >> + 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'"? Agreed >> + 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. As said in commit log, I'm thinking about having a common enough class for the list, or may be in another way, but it will be later patch, cleaning up vir{pci,usb}.[ch] together.