[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] rebased multipath patch



On Thu, Sep 03, 2009 at 06:03:00PM +0100, Daniel P. Berrange wrote:
> 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

 Okay, I will add this as a separate patch

> > 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.

  Added in

if WITH_STORAGE_MPATH
libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_MPATH_SOURCES)
libvirt_driver_storage_la_CFLAGS += $(DEVMAPPER_CFLAGS)
endif

> > +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.

  Done using the proper virStorageReportError()

> > +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;

  I also moved

    pool->def->capacity += vol->capacity;
    pool->def->allocation += vol->allocation;

just before exiting to avoid incleasing the capacity in case the
VIR_REALLOC_N(pool->volumes.objs) could fail.

>
>
> Allowing the removal of all the retval = -1 assignments
> during the method.
> 
> > +    }
> > +
> > +    if (!(strcmp(target_type, "multipath"))) {
>
> Can use STRNEQ() here

  I would assume that virStorageBackendIsMultipath() would
return 1 if target_type is actually "multipath", and hence
use
    if (STREQ(target_type, "multipath")) {
        ret = 1;
    }

ret being initialized to 0 at the beginning :-)

>
> ACK if those minor things are cleaned up

  I also fixed the Copyrights on the new code

  Applied and commited, thanks !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]