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

Re: [libvirt] [PATCH 2/2] Add fs pool formatting



On Thu, May 13, 2010 at 10:18:24PM -0400, David Allan wrote:
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index c96c4f3..c31f6a6 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
> 
> @@ -485,6 +486,83 @@ virStorageBackendFileSystemStart(virConnectPtr conn ATTRIBUTE_UNUSED,
>  #endif /* WITH_STORAGE_FS */
> 
> 
> +virStoragePoolProbeResult
> +virStorageBackendFileSystemProbeDummy(const char *device ATTRIBUTE_UNUSED,
> +                                      const char *format ATTRIBUTE_UNUSED)
> +{
> +    virStorageReportError(VIR_ERR_OPERATION_INVALID,
> +                          _("probing for filesystems is unsupported "
> +                            "by this build"));
> +
> +    return FILESYSTEM_PROBE_ERROR;
> +}

IMHO this doesn't really belong here.

> +static int
> +virStorageBackendMakeFileSystem(virStoragePoolObjPtr pool,
> +                                unsigned int flags)
> +{
> +    const char *device = NULL, *format = NULL;
> +    bool ok_to_mkfs = false;
> +    int ret = -1;
> +
> +    if (pool->def->source.devices == NULL) {
> +        virStorageReportError(VIR_ERR_OPERATION_INVALID,
> +                              _("No source device specified when formatting pool '%s'"),
> +                              pool->def->name);
> +        goto error;
> +    }
> +
> +    device = pool->def->source.devices[0].path;
> +    format = virStoragePoolFormatFileSystemTypeToString(pool->def->source.format);
> +    VIR_DEBUG("source device: '%s' format: '%s'", device, format);
> +
> +    if (!virFileExists(device)) {
> +        virStorageReportError(VIR_ERR_OPERATION_INVALID,
> +                              _("Source device does not exist when formatting pool '%s'"),
> +                              pool->def->name);
> +        goto error;
> +    }
> +
> +    if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) {
> +        ok_to_mkfs = true;
> +    } else if (flags & VIR_STORAGE_POOL_BUILD_PROBE &&
> +               virStorageBackendFileSystemProbe(device, format) ==
> +               FILESYSTEM_PROBE_NOT_FOUND) {
> +        ok_to_mkfs = true;
> +    }

This block needs to report an error if != FILESYSTEM_PROBE_NOT_FOUND

> +
> +    if (ok_to_mkfs) {
> +        ret = virStorageBackendExecuteMKFS(device, format);
> +    }
> +
> +error:
> +    return ret;
> +}
> +
>  /**
>   * @conn connection to report errors against
>   * @pool storage pool to build
> diff --git a/src/storage/storage_backend_fs.h b/src/storage/storage_backend_fs.h
> index 7def53e..d9e397f 100644
> --- a/src/storage/storage_backend_fs.h
> +++ b/src/storage/storage_backend_fs.h
> @@ -29,6 +29,25 @@
>  # if WITH_STORAGE_FS
>  extern virStorageBackend virStorageBackendFileSystem;
>  extern virStorageBackend virStorageBackendNetFileSystem;
> +
> +typedef enum {
> + FILESYSTEM_PROBE_FOUND,
> + FILESYSTEM_PROBE_NOT_FOUND,
> + FILESYSTEM_PROBE_ERROR,
> +} virStoragePoolProbeResult;
> +
> +#  if HAVE_LIBBLKID
> +#   define virStorageBackendFileSystemProbe virStorageBackendFileSystemProbeLibblkid
> +virStoragePoolProbeResult
> +virStorageBackendFileSystemProbeLibblkid(const char *device,
> +                                         const char *format);
> +#  else /* if HAVE_LIBBLKID */
> +#   define virStorageBackendFileSystemProbe virStorageBackendFileSystemProbeDummy
> +#  endif /* if HAVE_LIBBLKID */
> +virStoragePoolProbeResult
> +virStorageBackendFileSystemProbeDummy(const char *device,
> +                                      const char *format);
> +

This is rather gross

>  # endif
>  extern virStorageBackend virStorageBackendDirectory;
> 
> diff --git a/src/storage/storage_backend_fs_libblkid.c b/src/storage/storage_backend_fs_libblkid.c
> new file mode 100644
> index 0000000..ddd9264
> --- /dev/null
> +++ b/src/storage/storage_backend_fs_libblkid.c
> @@ -0,0 +1,97 @@
> +/*
> + * storage_backend_fs_libblkid.c: libblkid specific code for
> + * filesystem backend
> + *
> + * Copyright (C) 2007-2010 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307  USA
> + *
> + * Author: David Allan <dallan redhat com>
> + */
> +
> +#include <config.h>
> +#include <blkid/blkid.h>
> +
> +#include "virterror_internal.h"
> +#include "storage_backend_fs.h"
> +#include "logging.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_STORAGE
> +

Wrap this code in

#ifdef HAVE_LIBBLKID

> +virStoragePoolProbeResult
> +virStorageBackendFileSystemProbeLibblkid(const char *device,
> +                                         const char *format) {

s/virStorageBackendFileSystemProbeLibblkid/virStorageBackendFileSystemProbe/

> +
> +    virStoragePoolProbeResult ret = FILESYSTEM_PROBE_ERROR;
> +    blkid_probe probe = NULL;
> +    const char *fstype = NULL;
> +    char *names[2];
> +
> +    VIR_DEBUG("Probing for existing filesystem of type %s on device %s",
> +              format, device);
> +
> +    if (blkid_known_fstype(format) == 0) {
> +        virStorageReportError(VIR_ERR_STORAGE_PROBE_FAILED,
> +                              _("Not capable of probing for "
> +                                "filesystem of type %s"),
> +                              format);
> +        goto out;
> +    }
> +
> +    probe = blkid_new_probe_from_filename(device);
> +    if (probe == NULL) {
> +        virStorageReportError(VIR_ERR_STORAGE_PROBE_FAILED,
> +                                  _("Failed to create filesystem probe "
> +                                  "for device %s"),
> +                                  device);
> +        goto out;
> +    }
> +
> +    /* Unfortunately this value comes from the pool definition
> +     * where it is correctly const char, but liblbkid expects it
> +     * to be char, thus the cast. */
> +    names[0] = (char *)format;
> +    names[1] = NULL;
> +
> +    blkid_probe_filter_superblocks_type(probe,
> +                                        BLKID_FLTR_ONLYIN,
> +                                        names);
> +
> +    if (blkid_do_probe(probe) != 0) {
> +        VIR_INFO("No filesystem of type '%s' found on device '%s'",
> +                 format, device);
> +        ret = FILESYSTEM_PROBE_NOT_FOUND;
> +    } else if (blkid_probe_lookup_value(probe, "TYPE", &fstype, NULL) == 0) {
> +        virStorageReportError(VIR_ERR_STORAGE_PROBE_BUILT,
> +                              _("Existing filesystem of type '%s' found on "
> +                                "device '%s'"),
> +                              fstype, device);
> +        ret = FILESYSTEM_PROBE_FOUND;
> +    }
> +
> +    if (blkid_do_probe(probe) != 1) {
> +        virStorageReportError(VIR_ERR_STORAGE_PROBE_FAILED,
> +                                  _("Found additional probes to run, "
> +                                    "filesystem probing may be incorrect"));
> +        ret = FILESYSTEM_PROBE_ERROR;
> +    }
> +
> +out:
> +    if (probe != NULL) {
> +        blkid_free_probe(probe);
> +    }
> +
> +    return ret;
> +}

#else
 virStoragePoolProbeResult
 virStorageBackendFileSystemProbe(const char *device ATTRIBUTE_UNUSED,
                                  const char *format ATTRIBUTE_UNUSED)
 {
     virStorageReportError(VIR_ERR_OPERATION_INVALID,
                           _("probing for filesystems is unsupported "
                             "by this build"));
 
     return FILESYSTEM_PROBE_ERROR;
 }

#endif

> diff --git a/tools/virsh.c b/tools/virsh.c
> index 693d409..5f7804a 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -4668,6 +4668,9 @@ static const vshCmdInfo info_pool_build[] = {
> 
>  static const vshCmdOptDef opts_pool_build[] = {
>      {"pool", VSH_OT_DATA, VSH_OFLAG_REQ, N_("pool name or uuid")},
> +    {"probe", VSH_OT_BOOL, 0, N_("do not overwrite an existing "
> +                                 "pool of this type")},
> +    {"overwrite", VSH_OT_BOOL, 0, N_("overwrite any existing data")},
>      {NULL, 0, 0, NULL}
>  };

I'm not sure that 'probe' is a great name here. Perhaps '--no-overwrite'
is better. Likewise even for the include file constant name.

Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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