[libvirt] [PATCH 2/3] Format FS pools

[Daniel Veillard]

On Thu, Feb 18, 2010 at 05:58:06PM -0500, David Allan wrote:
> * If the user supplies the appropriate flag, create the filesystem on the partition used by the pool.
> ---
>  configure.ac                     |    5 +++++
>  include/libvirt/libvirt.h.in     |    3 ++-
>  src/storage/storage_backend_fs.c |   25 ++++++++++++++++++++++++-
>  3 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 743a357..616bd03 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1252,12 +1252,15 @@ AM_CONDITIONAL([WITH_STORAGE_DIR], [test "$with_storage_dir" = "yes"])
>  if test "$with_storage_fs" = "yes" -o "$with_storage_fs" = "check"; then
>    AC_PATH_PROG([MOUNT], [mount], [], [$PATH:/sbin:/usr/sbin])
>    AC_PATH_PROG([UMOUNT], [umount], [], [$PATH:/sbin:/usr/sbin])
> +  AC_PATH_PROG([MKE2FS], [mke2fs], [], [$PATH:/sbin:/usr/sbin])
>    if test "$with_storage_fs" = "yes" ; then
>      if test -z "$MOUNT" ; then AC_MSG_ERROR([We need mount for FS storage driver]) ; fi
>      if test -z "$UMOUNT" ; then AC_MSG_ERROR([We need umount for FS storage driver]) ; fi
> +    if test -z "$MKE2FS" ; then AC_MSG_ERROR([We need mke2fs for FS storage driver]) ; fi
>    else
>      if test -z "$MOUNT" ; then with_storage_fs=no ; fi
>      if test -z "$UMOUNT" ; then with_storage_fs=no ; fi
> +    if test -z "$MKE2FS" ; then with_storage_fs=no ; fi

  That's where things starts to be nasty, why ext2, and not say ext3,
then someone surely will want ext4 or xfs :-)
I'm afraid we probably need to implement those 4 and give them different
enums (below).

>      if test "$with_storage_fs" = "check" ; then with_storage_fs=yes ; fi
>    fi
> @@ -1268,6 +1271,8 @@ if test "$with_storage_fs" = "yes" -o "$with_storage_fs" = "check"; then
>          [Location or name of the mount program])
>      AC_DEFINE_UNQUOTED([UMOUNT],["$UMOUNT"],
>          [Location or name of the mount program])
> +    AC_DEFINE_UNQUOTED([MKE2FS],["$MKE2FS"],
> +        [Location or name of the mke2fs program])
>    fi
>  fi
>  AM_CONDITIONAL([WITH_STORAGE_FS], [test "$with_storage_fs" = "yes"])
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 260505e..a7751b8 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -1053,7 +1053,8 @@ typedef enum {
>  typedef enum {
>    VIR_STORAGE_POOL_BUILD_NEW  = 0,   /* Regular build from scratch */
>    VIR_STORAGE_POOL_BUILD_REPAIR = 1, /* Repair / reinitialize */
> -  VIR_STORAGE_POOL_BUILD_RESIZE = 2  /* Extend existing pool */
> +  VIR_STORAGE_POOL_BUILD_RESIZE = 2,  /* Extend existing pool */
> +  VIR_STORAGE_POOL_CREATE_FORMAT = 3  /* Format filesystem during build */

  Hum, I'm not so sure, CREATE_FORMAT == BUILD_REPAIR + BUILD_RESIZE
which after all sounds a plausible combination, I would use different
bits in the field, so set VIR_STORAGE_POOL_CREATE_FORMAT to 4.

  But now we need to cope with EXT3/EXT4/XFS variants, they should
probably be allocated on separate bits even if they are mutually
exclusive

>  } virStoragePoolBuildFlags;
> 
>  typedef enum {
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index bbd5787..eedbe2b 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -39,12 +39,14 @@
>  #include <libxml/xpath.h>
> 
>  #include "virterror_internal.h"
> +#include "internal.h"
>  #include "storage_backend_fs.h"
>  #include "storage_conf.h"
>  #include "storage_file.h"
>  #include "util.h"
>  #include "memory.h"
>  #include "xml.h"
> +#include "logging.h"
> 
>  #define VIR_FROM_THIS VIR_FROM_STORAGE
> 
> @@ -498,8 +500,9 @@ virStorageBackendFileSystemStart(virConnectPtr conn ATTRIBUTE_UNUSED,
>  static int
>  virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED,
>                                   virStoragePoolObjPtr pool,
> -                                 unsigned int flags ATTRIBUTE_UNUSED)
> +                                 unsigned int flags)
>  {
> +    const char *mke2fsargv[5], *device = NULL, *format = NULL;
>      int err, ret = -1;
>      char *parent;
>      char *p;
> @@ -540,6 +543,26 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED,
>                               pool->def->target.path);
>          goto error;
>      }
> +
> +    if (flags & VIR_STORAGE_POOL_CREATE_FORMAT) {

  We are really testing for the bit :-)

> +        device = pool->def->source.devices[0].path;
> +        format = virStoragePoolFormatFileSystemTypeToString(pool->def->source.format);
> +
> +        VIR_DEBUG("source device: '%s' format: '%s'", device, format);
> +
> +        mke2fsargv[0] = MKE2FS;
> +        mke2fsargv[1] = "-t";
> +        mke2fsargv[2] = format;
> +        mke2fsargv[3] = device;
> +        mke2fsargv[4] = NULL;
> +
> +        if (virRun(mke2fsargv, NULL) < 0) {
> +            VIR_ERROR("Failed to make '%s' filesystem on device '%s'",
> +                      format, device);
> +            goto error;
> +        }
> +    }
> +
>      ret = 0;
>  error:
>      VIR_FREE(parent);

  I think the core is there but we probably need to expand the format
options (detection at configure time sounds sufficient to me), most of
the code addition in virStorageBackendFileSystemBuild() can probably be
factorized for the diffferent formats. That should not be hard :-)

  thanks !

Daniel

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