6:14 p.m.
This patch adds a new backend to the storage driver supporting SCSI HBAs.
With this you can now enumerate physical disks provided by FibreChannel,
internal SCSI / PATA / SATA / USB adapters.
The implemention uses HAL to enumerate the volumes asssociated with an HBA.
It is common for most boxes to support multiple HBAs. eg on my Linux laptop
$ ls /sys/class/scsi_host/
host0 host1 host2 host3
These HBA names are used to identify the host when defining the XML:
<pool type="scsi">
<name>hba0</name>
<source>
<adapter name="host0"/>
</source>
<target>
<path>/dev/disk/by-path</path>
</target>
</pool>
This does not provide any means of discovering HBA names - that's a later
piece of broader work on host device discovery. It also does not support
multipath devices (eg with FibreChannel the same storage volume can appear
on the host multiple times)
Example using the first host adapter which is my internal SATA controller
with a single disk attached:
# ./src/virsh pool-create /root/hba0.xml
Pool hba0 created from /root/hba0.xml
# ./src/virsh vol-list hba0
Name Path
-----------------------------------------
0:0:0:0 /dev/disk/by-path/pci-0000:00:1f.2-scsi-0:0:0:0
# ./src/virsh vol-info /dev/disk/by-path/pci-0000:00:1f.2-scsi-0:0:0:0
Name: 0:0:0:0
Type: block
Capacity: 153.39 GB
Allocation: 153.39 GB
# ./src/virsh vol-dumpxml /dev/disk/by-path/pci-0000:00:1f.2-scsi-0:0:0:0
libvir: Storage error : no storage vol with matching key
<volume>
<name>0:0:0:0</name>
<key>SATA_HDT722516DLA380_VDK91GTE0WPLER</key>
<source>
</source>
<capacity>164696555520</capacity>
<allocation>164696555520</allocation>
<target>
<path>/dev/disk/by-path/pci-0000:00:1f.2-scsi-0:0:0:0</path>
<permissions>
<mode>060640</mode>
<owner>0</owner>
<group>6</group>
<label>system_u:object_r:fixed_disk_device_t</label>
</permissions>
</target>
</volume>
This also does not deal with the problem of HBAs being created on the fly,
eg the NPIV case where you can add/remove adapters at will. I'm still working
on how to deal with this. If you have your virtual NPIV adapters already
defined though, it can enumerate them fine.
Aside from the bit where I map from the short HBA name (eg 'host0') into the
HAL device name, this should be portable to Solaris as-is.
Dan.
--
|: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
6:15 p.m.
And this time with the patch included...
configure.in | 28 ++
src/Makefile.am | 10
src/storage_backend.c | 14 +
src/storage_backend_scsi.c | 466 +++++++++++++++++++++++++++++++++++++++++++++
src/storage_backend_scsi.h | 45 ++++
src/storage_conf.c | 9
src/util.c | 2
7 files changed, 571 insertions(+), 3 deletions(-)
Dan.
Index: configure.in
===================================================================
RCS file: /data/cvs/libvirt/configure.in,v
retrieving revision 1.136
diff -u -p -r1.136 configure.in
--- configure.in 21 Mar 2008 15:03:37 -0000 1.136
+++ configure.in 24 Mar 2008 23:05:25 -0000
@@ -28,6 +28,7 @@ GNUTLS_REQUIRED="1.0.25"
AVAHI_REQUIRED="0.6.0"
POLKIT_REQUIRED="0.6"
PARTED_REQUIRED="1.8.0"
+HAL_REQUIRED="0.5.0"
dnl Checks for C compiler.
AC_PROG_CC
@@ -584,6 +585,8 @@ AC_ARG_WITH(storage-iscsi,
[ --with-storage-iscsi with iSCSI backend for the storage driver
(on)],[],[with_storage_iscsi=check])
AC_ARG_WITH(storage-disk,
[ --with-storage-disk with GPartd Disk backend for the storage driver
(on)],[],[with_storage_disk=check])
+AC_ARG_WITH(storage-scsi,
+[ --with-storage-scsi with HAL SCSI Disk backend for the storage driver
(on)],[],[with_storage_scsi=check])
if test "$with_storage_fs" = "yes" -o "$with_storage_fs" =
"check"; then
AC_PATH_PROG(MOUNT, [mount], [], [$PATH:/sbin:/usr/sbin])
@@ -741,6 +744,30 @@ AC_SUBST(LIBPARTED_CFLAGS)
AC_SUBST(LIBPARTED_LIBS)
+HAL_CFLAGS=
+HAL_LIBS=
+if test "x$with_storage_scsi" = "xyes" -o
"x$with_storage_scsi" = "xcheck"; then
+ PKG_CHECK_MODULES(HAL, hal >= $HAL_REQUIRED,
+ [with_storage_scsi=yes], [
+ if test "x$with_storage_scsi" = "xcheck" ; then
+ with_storage_scsi=no
+ else
+ AC_MSG_ERROR(
+ [You must install HAL >= $HAL_REQUIRED to compile libvirt])
+ fi
+ ])
+ if test "x$with_storage_scsi" = "xyes" ; then
+ AC_DEFINE_UNQUOTED(WITH_STORAGE_SCSI_, 1,
+ [use SCSI backend for storage driver is enabled])
+ fi
+fi
+AC_DEFINE_UNQUOTED(WITH_STORAGE_SCSI, 1, [whether SCSI backend for storage driver is
enabled])
+AM_CONDITIONAL(WITH_STORAGE_SCSI, [test "yes" = "yes"])
+AC_SUBST(HAL_CFLAGS)
+AC_SUBST(HAL_LIBS)
+
+
+
dnl
dnl check for python
dnl
@@ -968,6 +995,7 @@ AC_MSG_NOTICE([ NetFS: $with_storage_f
AC_MSG_NOTICE([ LVM: $with_storage_lvm])
AC_MSG_NOTICE([ iSCSI: $with_storage_iscsi])
AC_MSG_NOTICE([ Disk: $with_storage_disk])
+AC_MSG_NOTICE([ SCSI: $with_storage_scsi])
AC_MSG_NOTICE([])
AC_MSG_NOTICE([Libraries])
AC_MSG_NOTICE([])
Index: src/Makefile.am
===================================================================
RCS file: /data/cvs/libvirt/src/Makefile.am,v
retrieving revision 1.75
diff -u -p -r1.75 Makefile.am
--- src/Makefile.am 21 Mar 2008 15:03:37 -0000 1.75
+++ src/Makefile.am 24 Mar 2008 23:05:25 -0000
@@ -9,6 +9,7 @@ INCLUDES = \
$(GNUTLS_CFLAGS) \
$(SASL_CFLAGS) \
$(SELINUX_CFLAGS) \
+ $(HAL_CFLAGS) \
-DBINDIR=\""$(libexecdir)"\" \
-DSBINDIR=\""$(sbindir)"\" \
-DSYSCONF_DIR="\"$(sysconfdir)\"" \
@@ -89,11 +90,18 @@ else
EXTRA_DIST += storage_backend_disk.h storage_backend_disk.c
endif
+if WITH_STORAGE_SCSI
+CLIENT_SOURCES += storage_backend_scsi.h storage_backend_scsi.c
+else
+EXTRA_DIST += storage_backend_scsi.h storage_backend_scsi.c
+endif
+
libvirt_la_SOURCES = $(CLIENT_SOURCES) $(SERVER_SOURCES)
-libvirt_la_LIBADD = $(LIBXML_LIBS) $(GNUTLS_LIBS) $(SASL_LIBS) $(SELINUX_LIBS) \
+libvirt_la_LIBADD = $(LIBXML_LIBS) $(GNUTLS_LIBS) $(SASL_LIBS) \
+ $(SELINUX_LIBS) $(HAL_LIBS) \
@CYGWIN_EXTRA_LIBADD@ ../gnulib/lib/libgnu.la
libvirt_la_LDFLAGS = -Wl,--version-script=$(srcdir)/libvirt_sym.version \
-version-info @LIBVIRT_VERSION_INFO@ \
Index: src/storage_backend.c
===================================================================
RCS file: /data/cvs/libvirt/src/storage_backend.c,v
retrieving revision 1.10
diff -u -p -r1.10 storage_backend.c
--- src/storage_backend.c 17 Mar 2008 16:57:21 -0000 1.10
+++ src/storage_backend.c 24 Mar 2008 23:05:25 -0000
@@ -45,6 +45,9 @@
#if WITH_STORAGE_DISK
#include "storage_backend_disk.h"
#endif
+#if WITH_STORAGE_SCSI
+#include "storage_backend_scsi.h"
+#endif
#include "util.h"
@@ -67,6 +70,9 @@ static virStorageBackendPtr backends[] =
#if WITH_STORAGE_DISK
&virStorageBackendDisk,
#endif
+#if WITH_STORAGE_SCSI
+ &virStorageBackendSCSI,
+#endif
};
@@ -121,6 +127,10 @@ virStorageBackendFromString(const char *
if (STREQ(type, "disk"))
return VIR_STORAGE_POOL_DISK;
#endif
+#if WITH_STORAGE_SCSI
+ if (STREQ(type, "scsi"))
+ return VIR_STORAGE_POOL_SCSI;
+#endif
virStorageReportError(NULL, VIR_ERR_INTERNAL_ERROR,
_("unknown storage backend type %s"), type);
@@ -150,6 +160,10 @@ virStorageBackendToString(int type) {
case VIR_STORAGE_POOL_DISK:
return "disk";
#endif
+#if WITH_STORAGE_SCSI
+ case VIR_STORAGE_POOL_SCSI:
+ return "scsi";
+#endif
}
virStorageReportError(NULL, VIR_ERR_INTERNAL_ERROR,
Index: src/storage_backend_scsi.c
===================================================================
RCS file: src/storage_backend_scsi.c
diff -N src/storage_backend_scsi.c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ src/storage_backend_scsi.c 24 Mar 2008 23:05:25 -0000
@@ -0,0 +1,466 @@
+/*
+ * storage_backend_scsi.c: storage backend for SCSI handling
+ *
+ * Copyright (C) 2007-2008 Red Hat, Inc.
+ * Copyright (C) 2007-2008 Daniel P. Berrange
+ *
+ * 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, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * Author: Daniel P. Berrange <berrange(a)redhat.com>
+ */
+
+#include <config.h>
+
+#include <ctype.h>
+#include <unistd.h>
+#include <hal/libhal.h>
+
+#include "storage_backend_scsi.h"
+
+#define LINUX_SYSFS_SCSI_HOST_PREFIX "/sys/class/scsi_host/"
+#define LINUX_SYSFS_SCSI_HOST_POSTFIX "/device"
+
+
+/**
+ * virStorageBackendSCSIMakeHostDevice:
+ * @conn: report errors agains
+ * @pool: pool to resolve
+ * @buf: pre-allocated buffer to fill with path name
+ * @buflen: size of buffer
+ *
+ * Resolve a HBA name to HBA device path
+ *
+ * Given a pool object configured for a SCSI HBA, resolve the HBA name
+ * into the canonical sysfs path for the HBA device. This can then be
+ * used to lookup the device in HAL.
+ *
+ * Returns 0 on success, -1 on failure
+ */
+static int virStorageBackendSCSIMakeHostDevice(virConnectPtr conn,
+ virStoragePoolObjPtr pool,
+ char *buf,
+ size_t buflen)
+{
+ char *dev = NULL;
+ char *tmp = pool->def->source.adapter;
+ char relLink[PATH_MAX], absLink[PATH_MAX];
+ ssize_t len;
+
+ if (!tmp) {
+ virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ "Missing adapter name for pool");
+ return -1;
+ }
+
+ /* Sanity check host adapter name - should only be 0-9, a-z.
+ * Anything else is bogus & so we reject it, to prevent them
+ * making us read arbitrary paths on host
+ */
+ while (*tmp) {
+ if (!isalnum(*tmp)) {
+ virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ "Invalid character in pool adapter name
'%s'",
+ pool->def->source.adapter);
+ return -1;
+ }
+ tmp++;
+ }
+
+ /*
+ * First get the class based path eg
+ *
+ * /sys/class/scsi_host/host1/device
+ */
+ if ((dev = malloc(sizeof(LINUX_SYSFS_SCSI_HOST_PREFIX) +
+ strlen(pool->def->source.adapter) +
+ sizeof(LINUX_SYSFS_SCSI_HOST_POSTFIX) +
+ 1)) == NULL) {
+ virStorageReportError(conn, VIR_ERR_NO_MEMORY,
+ "pool device");
+ return -1;
+ }
+
+ strcpy(dev, LINUX_SYSFS_SCSI_HOST_PREFIX);
+ strcat(dev, pool->def->source.adapter);
+ strcat(dev, LINUX_SYSFS_SCSI_HOST_POSTFIX);
+
+ /*
+ * Now resolve the class based path symlink to the real
+ * device path, which is likely under PCI bus hierarchy
+ * and is the path tracked by HAL
+ */
+ /* Readlink does not null terminate, so we reserve one byte */
+ if ((len = readlink(dev, relLink, sizeof(relLink)-1)) < 0) {
+ virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ "cannot find SCSI host adapter '%s' at
'%s'",
+ pool->def->source.adapter, dev);
+ free(dev);
+ return -1;
+ }
+ relLink[len] = '\0';
+
+
+ /*
+ * The symlink is relative, so now we have to turn it
+ * into a absolute path
+ */
+ if ((tmp = realloc(dev,
+ sizeof(LINUX_SYSFS_SCSI_HOST_PREFIX) +
+ strlen(pool->def->source.adapter) +
+ 1 +
+ strlen(relLink) +
+ 1)) == NULL) {
+ virStorageReportError(conn, VIR_ERR_NO_MEMORY,
+ "pool device");
+ free(dev);
+ return -1;
+ }
+ dev = tmp;
+
+ strcpy(dev, LINUX_SYSFS_SCSI_HOST_PREFIX);
+ strcat(dev, pool->def->source.adapter);
+ strcat(dev, "/");
+ strcat(dev, relLink);
+
+ /*
+ * And finally canonicalize the absolute path to remove the
+ * copious .. components
+ */
+ if (!realpath(dev, absLink)) {
+ virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ "cannot canonicalize link %s",
+ strerror(errno));
+ free(dev);
+ return -1;
+ }
+ free(dev);
+
+ if (strlen(absLink) >= buflen) {
+ virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ "link too long for buffer");
+ return -1;
+ }
+ strcpy(buf, absLink);
+
+ return 0;
+}
+
+
+/**
+ * virStorageBackendSCSICreateVol
+ *
+ * Allocate a virStorageVolDef object for the specified
+ * metadata and hook it into the pool
+ *
+ * Returns 0 on success, -1 on failure
+ */
+static int
+virStorageBackendSCSICreateVol(virConnectPtr conn,
+ virStoragePoolObjPtr pool,
+ const char *name,
+ const char *path,
+ const char *key,
+ unsigned long long size)
+{
+ virStorageVolDefPtr vol;
+ char *tmppath;
+
+ if ((vol = calloc(1, sizeof(*vol))) == NULL) {
+ virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s",
_("volume"));
+ goto cleanup;
+ }
+
+ tmppath = strdup(path);
+ vol->name = strdup(name);
+ vol->key = strdup(key);
+ vol->allocation = vol->capacity = size;
+
+ if ((vol->name == NULL) ||
+ (vol->key == NULL) ||
+ (tmppath == NULL)) {
+ virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s",
_("volume"));
+ goto cleanup;
+ }
+
+ /* Now figure out the stable path
+ *
+ * XXX this method is O(N) because it scans the pool target
+ * dir every time its run. Should figure out a more efficient
+ * way of doing this...
+ */
+ if ((vol->target.path = virStorageBackendStablePath(conn,
+ pool,
+ tmppath)) == NULL)
+ goto cleanup;
+
+ if (tmppath != vol->target.path)
+ free(tmppath);
+ tmppath = NULL;
+
+ if (virStorageBackendUpdateVolInfo(conn, vol, 0) < 0)
+ goto cleanup;
+
+ pool->def->capacity += vol->capacity;
+ pool->def->allocation += vol->allocation;
+
+ vol->next = pool->volumes;
+ pool->volumes = vol;
+ pool->nvolumes++;
+
+ return 0;
+
+ cleanup:
+ free(tmppath);
+ virStorageVolDefFree(vol);
+ return -1;
+}
+
+
+/**
+ * virStorageBackendSCSIAddLUN:
+ *
+ * @conn: connection to report errors against
+ * @pool: pool to register volume with
+ * @ctx: HAL context
+ * @devname: HAL device name for LUN
+ *
+ * Given a HAL device supported 'block' and 'storage' capabilities
+ * register it as a volume in the pool
+ *
+ * Returns 0 on success, -1 on error
+ */
+static int
+virStorageBackendSCSIAddLUN(virConnectPtr conn,
+ virStoragePoolObjPtr pool,
+ LibHalContext *ctx,
+ const char *devname)
+{
+ char **strdevs = NULL;
+ int numstrdevs = 0;
+ char *dev = NULL, *key = NULL;
+ unsigned long long size;
+ int host, bus, target, lun;
+ int n = -1, i;
+ int ret = -1;
+ char name[100];
+ DBusError derr = DBUS_ERROR_INIT;
+
+ if ((strdevs = libhal_manager_find_device_string_match(ctx,
+ "info.parent",
+ devname,
+ &numstrdevs,
+ &derr)) == NULL) {
+ virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ "failed to lookup LUNs for %s with HAL: %s",
+ devname, derr.message);
+ goto cleanup;
+ }
+ for (i = 0 ; i < numstrdevs && n == -1 ; i++) {
+ char *cat = libhal_device_get_property_string(ctx,
+ strdevs[0],
+ "info.category",
+ NULL);
+ /* XXX derr */
+ if (cat != NULL) {
+ if (STREQ(cat, "storage"))
+ n = i;
+ free(cat);
+ }
+ }
+ /* No storage vol for device - probably a removable vol so ignore */
+ if (n == -1) {
+ ret = 0;
+ goto cleanup;
+ }
+
+#define HAL_GET_PROPERTY(path, name, err, type, value) \
+ (value) = libhal_device_get_property_ ## type(ctx, (path), (name), (err)); \
+ if (dbus_error_is_set((err))) { \
+ virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, \
+ "unable to lookup '%s' property on %s:
%s", \
+ (name), (path), derr.message); \
+ goto cleanup; \
+ }
+
+ /* These are props of the physical device */
+ HAL_GET_PROPERTY(devname, "scsi.host", &derr, int, host);
+ HAL_GET_PROPERTY(devname, "scsi.bus", &derr, int, bus);
+ HAL_GET_PROPERTY(devname, "scsi.target", &derr, int, target);
+ HAL_GET_PROPERTY(devname, "scsi.lun", &derr, int, lun);
+
+ /* These are props of the logic device */
+ HAL_GET_PROPERTY(strdevs[0], "block.device", &derr, string, dev);
+ /*
+ * XXX storage.serial is not actually unique if they have
+ * multipath on the fibre channel adapter
+ */
+ HAL_GET_PROPERTY(strdevs[0], "storage.serial", &derr, string, key);
+ HAL_GET_PROPERTY(strdevs[0], "storage.size", &derr, uint64, size);
+
+#undef HAL_GET_PROPERTY
+
+ snprintf(name, sizeof(name), "%d:%d:%d:%d", host, bus, target, lun);
+ name[sizeof(name)-1] = '\0';
+
+ if (virStorageBackendSCSICreateVol(conn, pool, name, dev, key, size) < 0)
+ goto cleanup;
+ ret = 0;
+
+ cleanup:
+ for (i = 0 ; strdevs && i < numstrdevs ; i++)
+ free(strdevs[i]);
+ free(strdevs);
+ free(dev);
+ free(key);
+ if (dbus_error_is_set(&derr))
+ dbus_error_free(&derr);
+ return ret;
+}
+
+
+/**
+ * virStorageBackendSCSIRefreshPool:
+ *
+ * Query HAL for all storage devices associated with a specific
+ * SCSI HBA.
+ *
+ * XXX, currently we only support regular devices in /dev/,
+ * or /dev/disk/by-XXX. In future we also need to be able to
+ * map to multipath devices setup under /dev/mpath/XXXX.
+ */
+static int
+virStorageBackendSCSIRefreshPool(virConnectPtr conn,
+ virStoragePoolObjPtr pool)
+{
+ char hostdevice[PATH_MAX];
+ LibHalContext *ctx = NULL;
+ DBusConnection *sysbus = NULL;
+ DBusError derr = DBUS_ERROR_INIT;
+ char **hbadevs = NULL, **blkdevs = NULL;
+ int numhbadevs = 0, numblkdevs = 0;
+ int i, ret = -1;
+
+ /* Get the canonical sysfs path for the HBA device */
+ if (virStorageBackendSCSIMakeHostDevice(conn, pool,
+ hostdevice, sizeof(hostdevice)) < 0)
+ goto cleanup;
+
+ if ((ctx = libhal_ctx_new()) == NULL) {
+ virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ "failed to allocate HAL context");
+ goto cleanup;
+ }
+
+ sysbus = dbus_bus_get(DBUS_BUS_SYSTEM, &derr);
+ if (sysbus == NULL) {
+ virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ "failed to get dbus system bus: %s",
+ derr.message);
+ goto cleanup;
+ }
+
+ if (!libhal_ctx_set_dbus_connection(ctx, sysbus)) {
+ virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ "failed to set HAL dbus connection");
+ goto cleanup;
+ }
+
+
+ if (!libhal_ctx_init(ctx, &derr)) {
+ virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ "failed to initialize HAL context: %s",
+ derr.message);
+ goto cleanup;
+ }
+
+ if ((hbadevs = libhal_manager_find_device_string_match(ctx,
+ "linux.sysfs_path",
+ hostdevice,
+ &numhbadevs,
+ &derr)) == NULL) {
+ virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ "failed to lookup device %s with HAL: %s",
+ hostdevice, derr.message);
+ goto cleanup;
+ }
+
+ if (numhbadevs != 1) {
+ virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ "unexpected number of HBA devices from HAL");
+ goto cleanup;
+ }
+
+ if ((blkdevs = libhal_manager_find_device_string_match(ctx,
+ "info.parent",
+ hbadevs[0],
+ &numblkdevs,
+ &derr)) == NULL) {
+ virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ "failed to lookup LUNs for %s with HAL: %s",
+ hbadevs[0], derr.message);
+ goto cleanup;
+ }
+
+ for (i = 0 ; i < numblkdevs ; i++)
+ virStorageBackendSCSIAddLUN(conn,
+ pool,
+ ctx,
+ blkdevs[i]);
+
+ ret = 0;
+
+ cleanup:
+ for (i = 0 ; hbadevs && i < numhbadevs ; i++)
+ free(hbadevs[i]);
+ free(hbadevs);
+ for (i = 0 ; blkdevs && i < numblkdevs ; i++)
+ free(blkdevs[i]);
+ free(blkdevs);
+ libhal_ctx_shutdown(ctx, NULL);
+ libhal_ctx_free(ctx);
+ if (sysbus)
+ dbus_connection_unref(sysbus);
+ if (dbus_error_is_set(&derr))
+ dbus_error_free(&derr);
+ return ret;
+}
+
+
+
+virStorageBackend virStorageBackendSCSI = {
+ .type = VIR_STORAGE_POOL_SCSI,
+
+ .refreshPool = virStorageBackendSCSIRefreshPool,
+
+ .poolOptions = {
+ .flags = (VIR_STORAGE_BACKEND_POOL_SOURCE_ADAPTER)
+ },
+
+ .volType = VIR_STORAGE_VOL_BLOCK,
+};
+
+/*
+ * vim: set tabstop=4:
+ * vim: set shiftwidth=4:
+ * vim: set expandtab:
+ */
+/*
+ * Local variables:
+ * indent-tabs-mode: nil
+ * c-indent-level: 4
+ * c-basic-offset: 4
+ * tab-width: 4
+ * End:
+ */
Index: src/storage_backend_scsi.h
===================================================================
RCS file: src/storage_backend_scsi.h
diff -N src/storage_backend_scsi.h
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ src/storage_backend_scsi.h 24 Mar 2008 23:05:25 -0000
@@ -0,0 +1,45 @@
+/*
+ * storage_backend_scsi.h: storage backend for SCSI handling
+ *
+ * Copyright (C) 2007-2008 Red Hat, Inc.
+ * Copyright (C) 2007-2008 Daniel P. Berrange
+ *
+ * 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, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * Author: Daniel P. Berrange <berrange(a)redhat.com>
+ */
+
+#ifndef __VIR_STORAGE_BACKEND_SCSI_H__
+#define __VIR_STORAGE_BACKEND_SCSI_H__
+
+#include "storage_backend.h"
+
+extern virStorageBackend virStorageBackendSCSI;
+
+#endif /* __VIR_STORAGE_BACKEND_SCSI_H__ */
+
+/*
+ * vim: set tabstop=4:
+ * vim: set shiftwidth=4:
+ * vim: set expandtab:
+ */
+/*
+ * Local variables:
+ * indent-tabs-mode: nil
+ * c-indent-level: 4
+ * c-basic-offset: 4
+ * tab-width: 4
+ * End:
+ */
Index: src/storage_conf.c
===================================================================
RCS file: /data/cvs/libvirt/src/storage_conf.c,v
retrieving revision 1.3
diff -u -p -r1.3 storage_conf.c
--- src/storage_conf.c 27 Feb 2008 10:37:19 -0000 1.3
+++ src/storage_conf.c 24 Mar 2008 23:05:25 -0000
@@ -97,6 +97,7 @@ virStoragePoolDefFree(virStoragePoolDefP
}
free(def->source.devices);
free(def->source.dir);
+ free(def->source.adapter);
if (def->source.authType == VIR_STORAGE_POOL_AUTH_CHAP) {
free(def->source.auth.chap.login);
@@ -320,6 +321,14 @@ virStoragePoolDefParseDoc(virConnectPtr
}
}
+ if (options->flags & VIR_STORAGE_BACKEND_POOL_SOURCE_ADAPTER) {
+ if ((ret->source.adapter =
virXPathString("string(/pool/source/adapter/@name)", ctxt)) == NULL) {
+ virStorageReportError(conn, VIR_ERR_XML_ERROR,
+ "%s", _("missing source
adapter"));
+ goto cleanup;
+ }
+ }
+
authType = virXPathString("string(/pool/source/auth/@type)", ctxt);
if (authType == NULL) {
Index: src/util.c
===================================================================
RCS file: /data/cvs/libvirt/src/util.c,v
retrieving revision 1.26
diff -u -p -r1.26 util.c
--- src/util.c 20 Mar 2008 11:24:30 -0000 1.26
+++ src/util.c 24 Mar 2008 23:05:25 -0000
@@ -443,8 +443,6 @@ int virFileLinkPointsTo(const char *chec
/* compare */
if (strcmp(checkReal, real) != 0) {
- virLog("Link '%s' does not point to '%s', ignoring",
- checkLink, checkReal);
return 0;
}
--
|: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
2:24 a.m.
On Mon, Mar 24, 2008 at 11:15:47PM +0000, Daniel P. Berrange wrote:
And this time with the patch included...
[...]
PARTED_REQUIRED="1.8.0"
+HAL_REQUIRED="0.5.0"
With HAL being in Solaris now, maybe that could even be made non-linux
specific
[...]
@@ -584,6 +585,8 @@ AC_ARG_WITH(storage-iscsi,
[ --with-storage-iscsi with iSCSI backend for the storage driver
(on)],[],[with_storage_iscsi=check])
AC_ARG_WITH(storage-disk,
[ --with-storage-disk with GPartd Disk backend for the storage driver
(on)],[],[with_storage_disk=check])
+AC_ARG_WITH(storage-scsi,
+[ --with-storage-scsi with HAL SCSI Disk backend for the storage driver
(on)],[],[with_storage_scsi=check])
if test "$with_storage_fs" = "yes" -o "$with_storage_fs" =
"check"; then
AC_PATH_PROG(MOUNT, [mount], [], [$PATH:/sbin:/usr/sbin])
@@ -741,6 +744,30 @@ AC_SUBST(LIBPARTED_CFLAGS)
AC_SUBST(LIBPARTED_LIBS)
maybe a meta option --with-storage giving a default to the group of options
(that now 5 of them) could be helpful for reduced setups.
[...]
diff -N src/storage_backend_scsi.c
[...]
+#define LINUX_SYSFS_SCSI_HOST_PREFIX
"/sys/class/scsi_host/"
+#define LINUX_SYSFS_SCSI_HOST_POSTFIX "/device"
hopefully we can add Solaris equivalent later
+ if (strlen(absLink) >= buflen) {
+ virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ "link too long for buffer");
+ return -1;
+ }
+ strcpy(buf, absLink);
Even if just checked can we still use strncpy(), i assume at some point
we will add an automatic check against strcpy
[...]
+virStorageBackendSCSICreateVol(virConnectPtr conn,
+ virStoragePoolObjPtr pool,
+ const char *name,
+ const char *path,
+ const char *key,
+ unsigned long long size)
+{
+ virStorageVolDefPtr vol;
+ char *tmppath;
+
+ if ((vol = calloc(1, sizeof(*vol))) == NULL) {
+ virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s",
_("volume"));
+ goto cleanup;
+ }
maybe check that size is greater than some absolute minimum like 1 MByte
(and maximum size ?) this could also allow to catch some conversion errors
or 0 being passed.
+ tmppath = strdup(path);
[...]
+ if ((vol->target.path = virStorageBackendStablePath(conn,
+ pool,
+ tmppath)) == NULL)
+ goto cleanup;
+
+ if (tmppath != vol->target.path)
+ free(tmppath);
+ tmppath = NULL;
it's a bit funky, you mean virStorageBackendStablePath could just grab the
parameter string or not and that need to be checked later to decide if it needs
freeing ? Even for an internal API it's a bit dangerous IMHO, what if
it is called with a constant string path, let's avoid the potential problem
get virStorageBackendStablePath to always strdup if it reuses the parameter,
no ?
[...]
+ goto cleanup;
+ }
+
+#define HAL_GET_PROPERTY(path, name, err, type, value) \
+ (value) = libhal_device_get_property_ ## type(ctx, (path), (name), (err)); \
+ if (dbus_error_is_set((err))) { \
+ virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, \
+ "unable to lookup '%s' property on %s:
%s", \
+ (name), (path), derr.message); \
+ goto cleanup; \
+ }
i'm not too fond of macros defined in the function code, move it just before
Also for token-pasting I though one needed to glue the # and the identifiers
like foo##bar , i'm surprized foo ## bar actually works
+ /* These are props of the physical device */
+ HAL_GET_PROPERTY(devname, "scsi.host", &derr, int, host);
+ HAL_GET_PROPERTY(devname, "scsi.bus", &derr, int, bus);
+ HAL_GET_PROPERTY(devname, "scsi.target", &derr, int, target);
+ HAL_GET_PROPERTY(devname, "scsi.lun", &derr, int, lun);
+
+ /* These are props of the logic device */
+ HAL_GET_PROPERTY(strdevs[0], "block.device", &derr, string, dev);
+ /*
+ * XXX storage.serial is not actually unique if they have
+ * multipath on the fibre channel adapter
+ */
+ HAL_GET_PROPERTY(strdevs[0], "storage.serial", &derr, string, key);
+ HAL_GET_PROPERTY(strdevs[0], "storage.size", &derr, uint64, size);
+
+#undef HAL_GET_PROPERTY
Looks good, and will make easier to test the simple storage management
on developpers machines, +1
thanks !
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
11:12 a.m.
On Tue, Mar 25, 2008 at 03:24:48AM -0400, Daniel Veillard wrote:
On Mon, Mar 24, 2008 at 11:15:47PM +0000, Daniel P. Berrange wrote:
> And this time with the patch included...
[...]
> PARTED_REQUIRED="1.8.0"
> +HAL_REQUIRED="0.5.0"
With HAL being in Solaris now, maybe that could even be made non-linux
specific
[...]
> @@ -584,6 +585,8 @@ AC_ARG_WITH(storage-iscsi,
> [ --with-storage-iscsi with iSCSI backend for the storage driver
(on)],[],[with_storage_iscsi=check])
> AC_ARG_WITH(storage-disk,
> [ --with-storage-disk with GPartd Disk backend for the storage driver
(on)],[],[with_storage_disk=check])
> +AC_ARG_WITH(storage-scsi,
> +[ --with-storage-scsi with HAL SCSI Disk backend for the storage driver
(on)],[],[with_storage_scsi=check])
>
> if test "$with_storage_fs" = "yes" -o
"$with_storage_fs" = "check"; then
> AC_PATH_PROG(MOUNT, [mount], [], [$PATH:/sbin:/usr/sbin])
> @@ -741,6 +744,30 @@ AC_SUBST(LIBPARTED_CFLAGS)
> AC_SUBST(LIBPARTED_LIBS)
maybe a meta option --with-storage giving a default to the group of options
(that now 5 of them) could be helpful for reduced setups.
Nah, you never need to use any of the --with-storage-XXX options in
normal circumstances. If you leave them out, it will automatically
probe and enable/disable as appropriate. You only need to use the
--with options if you want to force configure to abort if probing
fails.
[...]
> diff -N src/storage_backend_scsi.c
[...]
> +#define LINUX_SYSFS_SCSI_HOST_PREFIX "/sys/class/scsi_host/"
> +#define LINUX_SYSFS_SCSI_HOST_POSTFIX "/device"
hopefully we can add Solaris equivalent later
Yeah, or change the way the Linux bit works so it sucks less. I'm
still undecided on this particular bit of code. I'm using sysfs
because I'll need to use it anyway when I add NPIV support. Then
again NPIV support may work out differently to how I expect - I'll
be getting access to some real NPIV hardware soon to verify...
> + if (strlen(absLink) >= buflen) {
> + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
> + "link too long for buffer");
> + return -1;
> + }
> + strcpy(buf, absLink);
Even if just checked can we still use strncpy(), i assume at some point
we will add an automatic check against strcpy
Sure, will do.
[...]
> +virStorageBackendSCSICreateVol(virConnectPtr conn,
> + virStoragePoolObjPtr pool,
> + const char *name,
> + const char *path,
> + const char *key,
> + unsigned long long size)
> +{
> + virStorageVolDefPtr vol;
> + char *tmppath;
> +
> + if ((vol = calloc(1, sizeof(*vol))) == NULL) {
> + virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s",
_("volume"));
> + goto cleanup;
> + }
maybe check that size is greater than some absolute minimum like 1 MByte
(and maximum size ?) this could also allow to catch some conversion errors
or 0 being passed.
This size value comes straight from HAL, as is the LUN size. The code
will re-probe this value a short while later when we open the actual
device node to lookup permissions/ownership, so I think its overkill
to check size here too.
> + tmppath = strdup(path);
[...]
> + if ((vol->target.path = virStorageBackendStablePath(conn,
> + pool,
> + tmppath)) == NULL)
> + goto cleanup;
> +
> + if (tmppath != vol->target.path)
> + free(tmppath);
> + tmppath = NULL;
it's a bit funky, you mean virStorageBackendStablePath could just grab the
parameter string or not and that need to be checked later to decide if it needs
freeing ? Even for an internal API it's a bit dangerous IMHO, what if
it is called with a constant string path, let's avoid the potential problem
get virStorageBackendStablePath to always strdup if it reuses the parameter,
no ?
Yeah, guess I should change the contract of that method. Its a little ugly.
All the other drivers work in this way though, so I'll do that cleanup as
a separate patch.
> + goto cleanup;
> + }
> +
> +#define HAL_GET_PROPERTY(path, name, err, type, value) \
> + (value) = libhal_device_get_property_ ## type(ctx, (path), (name), (err)); \
> + if (dbus_error_is_set((err))) { \
> + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, \
> + "unable to lookup '%s' property on %s:
%s", \
> + (name), (path), derr.message); \
> + goto cleanup; \
> + }
i'm not too fond of macros defined in the function code, move it just before
Also for token-pasting I though one needed to glue the # and the identifiers
like foo##bar , i'm surprized foo ## bar actually works
Works for GCC at least, but it could be that's a non-portable GCC extension
Dan.
--
|: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
6085
days inactive
6086
days old
3 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Daniel Veillard