[libvirt] [PATCH 03/25] utils: util functions for scsi hostdev
Osier Yang
jyang at redhat.com
Mon May 13 10:43:29 UTC 2013
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 at cn.fujitsu.com>
>>>>
>>>> This patch adds util functions for scsi hostdev.
>>>>
>>>> Signed-off-by: Han Cheng <hanc.fnst at cn.fujitsu.com>
>>>> Signed-off-by: Osier Yang <jyang at 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 at 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.
More information about the libvir-list
mailing list