On Wed, Sep 02, 2009 at 11:28:27AM -0400, Dave Allan wrote:
> @@ -1177,6 +1180,26 @@ if test "$with_storage_scsi" = "check"; then
> fi
> AM_CONDITIONAL([WITH_STORAGE_SCSI], [test "$with_storage_scsi" = "yes"])
>
> +if test "$with_storage_mpath" = "check"; then
> + with_storage_mpath=yes
> +
> + AC_DEFINE_UNQUOTED([WITH_STORAGE_MPATH], 1,
> + [whether mpath backend for storage driver is enabled])
> +fi
> +AM_CONDITIONAL([WITH_STORAGE_MPATH], [test "$with_storage_mpath" = "yes"])
> +
> +if test "$with_storage_mpath" = "yes"; then
> + DEVMAPPER_REQUIRED=0.0
> + DEVMAPPER_CFLAGS=
> + DEVMAPPER_LIBS=
> + PKG_CHECK_MODULES(DEVMAPPER, devmapper >= $DEVMAPPER_REQUIRED,
> + [], [
> + AC_MSG_ERROR(
> + [You must install device-mapper-devel >= $DEVMAPPER_REQUIRED to compile libvirt])
> + ])
> +fi
> +AC_SUBST([DEVMAPPER_CFLAGS])
> +AC_SUBST([DEVMAPPER_LIBS])
Need to update livbvirt.spec.in with a dependancy on device mapper for this
tool, both Requires & BuildRequries
> diff --git a/src/Makefile.am b/src/Makefile.am
> index bedeb84..cf3420b 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -199,6 +199,9 @@ STORAGE_DRIVER_ISCSI_SOURCES = \
> STORAGE_DRIVER_SCSI_SOURCES = \
> storage_backend_scsi.h storage_backend_scsi.c
>
> +STORAGE_DRIVER_MPATH_SOURCES = \
> + storage_backend_mpath.h storage_backend_mpath.c
> +
> STORAGE_DRIVER_DISK_SOURCES = \
> storage_backend_disk.h storage_backend_disk.c
>
> @@ -478,6 +481,10 @@ if WITH_STORAGE_SCSI
> libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_SCSI_SOURCES)
> endif
>
> +if WITH_STORAGE_MPATH
> +libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_MPATH_SOURCES)
> +endif
> +
> if WITH_STORAGE_DISK
> libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_DISK_SOURCES)
> endif
> @@ -539,6 +546,7 @@ EXTRA_DIST += \
> $(STORAGE_DRIVER_LVM_SOURCES) \
> $(STORAGE_DRIVER_ISCSI_SOURCES) \
> $(STORAGE_DRIVER_SCSI_SOURCES) \
> + $(STORAGE_DRIVER_MPATH_SOURCES) \
> $(STORAGE_DRIVER_DISK_SOURCES) \
> $(NODE_DEVICE_DRIVER_SOURCES) \
> $(NODE_DEVICE_DRIVER_HAL_SOURCES) \
> @@ -607,6 +615,7 @@ libvirt_la_LDFLAGS = $(VERSION_SCRIPT_FLAGS)libvirt.syms \
> $(COVERAGE_CFLAGS:-f%=-Wc,-f%) \
> $(LIBXML_LIBS) $(SELINUX_LIBS) \
> $(XEN_LIBS) $(DRIVER_MODULE_LIBS) \
> + $(DEVMAPPER_LIBS) \
> @CYGWIN_EXTRA_LDFLAGS@ @MINGW_EXTRA_LDFLAGS@
> libvirt_la_CFLAGS = $(COVERAGE_CFLAGS) -DIN_LIBVIRT
> libvirt_la_DEPENDENCIES = $(libvirt_la_LIBADD) libvirt.syms
I think you probably need a $(DEVMAPPER_CFLAGS) somewhere in here too,
even if it happens to be empty for default Fedora installs.
> +static int
> +virStorageBackendMpathUpdateVolTargetInfo(virConnectPtr conn,
> + virStorageVolTargetPtr target,
> + unsigned long long *allocation,
> + unsigned long long *capacity)
> +{
> + int ret = 0;
> + int fd = -1;
> +
> + if ((fd = open(target->path, O_RDONLY)) < 0) {
> + virReportSystemError(conn, errno,
> + _("cannot open volume '%s'"),
> + target->path);
> + ret = -1;
> + goto out;
> + }
> +
> + if (virStorageBackendUpdateVolTargetInfoFD(conn,
> + target,
> + fd,
> + allocation,
> + capacity) < 0) {
> +
> + VIR_ERROR(_("Failed to update volume target info for %s"),
> + target->path);
> +
> + ret = -1;
> + goto out;
> + }
> +
> + if (virStorageBackendUpdateVolTargetFormatFD(conn,
> + target,
> + fd) < 0) {
> + VIR_ERROR(_("Failed to update volume target format for %s"),
> + target->path);
> +
> + ret = -1;
> + goto out;
> + }
These two VIR_EROR calls should be virRaiseError() or similar
if we're going to return a fail status.
> +static int
> +virStorageBackendMpathNewVol(virConnectPtr conn,
> + virStoragePoolObjPtr pool,
> + const int devnum,
> + const char *dev)
> +{
> + virStorageVolDefPtr vol;
> + int retval = 0;
> +
> + if (VIR_ALLOC(vol) < 0) {
> + virReportOOMError(conn);
> + retval = -1;
> + goto out;
> + }
> +
> + vol->type = VIR_STORAGE_VOL_BLOCK;
> +
> + if (virAsprintf(&(vol->name), "dm-%u", devnum) < 0) {
> + virReportOOMError(conn);
> + retval = -1;
> + goto free_vol;
> + }
> +
> + if (virAsprintf(&vol->target.path, "/dev/%s", dev) < 0) {
> + virReportOOMError(conn);
> + retval = -1;
> + goto free_vol;
> + }
> +
> + if (virStorageBackendMpathUpdateVolTargetInfo(conn,
> + &vol->target,
> + &vol->allocation,
> + &vol->capacity) < 0) {
> +
> + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
> + _("Failed to update volume for '%s'"),
> + vol->target.path);
> + retval = -1;
> + goto free_vol;
> + }
> +
> + /* XXX should use logical unit's UUID instead */
> + vol->key = strdup(vol->target.path);
> + if (vol->key == NULL) {
> + virReportOOMError(conn);
> + retval = -1;
> + goto free_vol;
> + }
> +
> + pool->def->capacity += vol->capacity;
> + pool->def->allocation += vol->allocation;
> +
> + if (VIR_REALLOC_N(pool->volumes.objs,
> + pool->volumes.count + 1) < 0) {
> + virReportOOMError(conn);
> + retval = -1;
> + goto free_vol;
> + }
> + pool->volumes.objs[pool->volumes.count++] = vol;
> +
If 'retval' is declared as -1 initially, then
> + goto out;
> +
> +free_vol:
> + virStorageVolDefFree(vol);
> +out:
> + return retval;
> +}
could be simplified with
ret = 0;
cleanup:
if (ret != 0)
virStorageVolDefFree(vol);
return ret;
Allowing the removal of all the retval = -1 assignments
during the method.
> + }
> +
> + if (!(strcmp(target_type, "multipath"))) {
Can use STRNEQ() here
ACK if those minor things are cleaned up
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