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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/