5:43 a.m.
On 13/05/13 18:42, Osier Yang wrote:
On 07/05/13 19:09, Osier Yang wrote:
> 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>
>>>
>>> This patch adds util functions for scsi hostdev.
>>>
>>> Signed-off-by: Han Cheng <hanc.fnst(a)cn.fujitsu.com>
>>> Signed-off-by: Osier Yang <jyang(a)redhat.com>
>>>
>>> ---
>>> 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>
>>> + */
>>> +
>>> +#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.
>
Pushed with the ACK'ed nits fixed.
With 15/25 merged.