Re: [libvirt] rebased multipath patch

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