Re: [libvirt] [PATCH 1/1] Format FS pools

On 02/23/2010 03:15 AM, Daniel Veillard wrote: >On Mon, Feb 22, 2010 at 01:44:39PM -0500, David Allan wrote: >>* Create the filesystem on the partition used by the pool. >>--- >> configure.ac | 5 +++++ >> src/storage/storage_backend_fs.c | 22 ++++++++++++++++++++++ >> 2 files changed, 27 insertions(+), 0 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]) > > Actually I think we need to check for mkfs to be filesystem generic > >> 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 >> >> 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/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c >>index 8279d78..eb5da5e 100644 >>--- a/src/storage/storage_backend_fs.c >>+++ b/src/storage/storage_backend_fs.c >>@@ -45,6 +45,7 @@ >> #include "util.h" >> #include "memory.h" >> #include "xml.h" >>+#include "logging.h" >> >> #define VIR_FROM_THIS VIR_FROM_STORAGE >> >>@@ -500,6 +501,7 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, >> virStoragePoolObjPtr pool, >> unsigned int flags ATTRIBUTE_UNUSED) >> { >>+ const char *mke2fsargv[5], *device = NULL, *format = NULL; >> int err, ret = -1; >> char *parent; >> char *p; >>@@ -540,6 +542,26 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, >> pool->def->target.path); >> goto error; >> } >>+ >>+ 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"; > > mkfs has a -t type argument, not mke2fs on my machine > >>+ mke2fsargv[2] = format; >>+ mke2fsargv[3] = device; >>+ mke2fsargv[4] = NULL; >>+ >>+ if (virRun(mke2fsargv, NULL)< 0) { >>+ virReportSystemError(errno, >>+ _("Failed to make filesystem of " >>+ "type '%s' on device '%s'"), >>+ format, device); >>+ goto error; >>+ } >>+ >> ret = 0; >> error: >> VIR_FREE(parent); > > So I come up with the following patch instead. Note that /sbin/mkfs >is provided by util-linux which is alread required for the storage fs >support so we don't need to add anything (BTW util-linux is now provided >by util-linux-ng on recent Fedoras, but keeping the old canonical >require name is probably better). > >Daniel > ACK. Tested with ext3, ext4 and xfs.