On Fri, Feb 20, 2009 at 05:14:55PM -0500, David Allan wrote:
> Last March, DanPB posted code to create pools from SCSI HBAs. This patch is simply bringing that code into the current tree.
>
> * src/storage_backend_scsi.[ch] contain the pool implementataion
> * There are small changes to several other source files to integrate the new pool type.
> @@ -876,6 +879,13 @@ if test "$with_storage_iscsi" = "yes" -o "$with_storage_iscsi" = "check"; then
> fi
> AM_CONDITIONAL([WITH_STORAGE_ISCSI], [test "$with_storage_iscsi" = "yes"])
>
> +if test "$with_storage_scsi" = "check"; then
> + with_storage_scsi=yes
> +
> + AC_DEFINE_UNQUOTED([WITH_STORAGE_SCSI], 1,
> + [whether SCSI backend for storage driver is enabled])
> + AM_CONDITIONAL([WITH_STORAGE_SCSI], [test "$with_storage_scsi" = "yes"])
> +fi
This isn't quite right, because the code depends on HAL but we're
not checking for it here.
There are other checks in configure.ac that look for HAL, but they
are not neccessarily enabled based on the same flags. But see my
comments later about whether we should just avoid HAL....
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 3a798d2..f1dd789 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -9,6 +9,7 @@ INCLUDES = \
> $(XEN_CFLAGS) \
> $(SELINUX_CFLAGS) \
> $(DRIVER_MODULE_CFLAGS) \
> + $(POLKIT_CFLAGS) \
This isn't related to HAL or SCSI drivers.
> +/**
> + * 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)
> +{
In this method we basically have a HBA number, and ask HAL for
all devices which are children of this. We still have a whole
bunch of code in the virStorageBackendSCSIAddLUN() which is
called per device we find, that pokes around in sysfs to get
out more metadata. We also have similar, but different code
in the iSCSI code that pokes around in sysfs.
Further more HAL is deprecated for dealing with storage devices
in Fedora 10 and later, with DeviceKit targetted to take over
its role. It is a bit of a mess really. I'm inclined to say
thta we should not use HAL for this SCSI stuff at all, and
instead we should slightly generalize our existing iSCSI method
virStorageBackendISCSIFindLUNs() so that it works for both the
SCSI and iSCSI storage pools, letting us share the code.
> + 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,
> + _("HAL failed to lookup device '%s': %s"),
> + hostdevice, derr.message);
> + goto cleanup;
> + }
> +
> + if (numhbadevs != 1) {
> + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
> + _("unexpected number of HBA devices from HAL "
> + "(expected 1, got %d) when looking up device "
> + "'%s'"), numhbadevs, hostdevice);
> + 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,
> +};
> +
> +/*
> + * 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:
> + */
We've since decided against adding these editor footers.
The virStorageBackendISCSIFindLUNs() method currently takes an iSCSI
session name and uses that to find the host:bus:target prefix for the
virtual iSCSI HBA, then scans for LUNS with that prefix.
If we split this into 2 methods, the first
virStorageBackendISCSIFindLUNsForSession()
Just does the session -> host:bus:target lookup, then calling
virStorageBackendSCSIFindLUNs(int host, int bus, int target)
which does the scan matching /sys/bus/scsi/host:bus:target:lun to find
the LUNs. Then, this new SCSI pool would merely need a short piece of
code to call virStorageBackendSCSIFindLUNs() and avoid all this HAL
stuff.
Daniel
--
|: Red Hat, Engineering, London -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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list