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

Re: [libvirt] [PATCH 6/7] Break out FS volume build routines to their own functions.



On Mon, May 04, 2009 at 01:43:01PM -0400, Cole Robinson wrote:
> Improves readability, particularly wrt the pending CreateFromXML changes.
> This will also help implementing File->Block volume creation in the future,
> since some of this code will need to be moved to a backend agnostic file.

This is a nice idea - it was getting rather unwieldly as it grew.

ACK.

Daniel

> 
> Signed-off-by: Cole Robinson <crobinso redhat com>
> ---
>  src/storage_backend_fs.c |  327 +++++++++++++++++++++++++---------------------
>  1 files changed, 179 insertions(+), 148 deletions(-)
> 
> diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c
> index 7a1bba8..241fb29 100644
> --- a/src/storage_backend_fs.c
> +++ b/src/storage_backend_fs.c
> @@ -62,6 +62,8 @@ static int qcowXGetBackingStore(virConnectPtr, char **,
>  static int vmdk4GetBackingStore(virConnectPtr, char **,
>                                  const unsigned char *, size_t);
>  
> +typedef int (*createFile)(virConnectPtr conn, virStorageVolDefPtr vol);
> +
>  static int track_allocation_progress = 0;
>  
>  /* Either 'magic' or 'extension' *must* be provided */
> @@ -1011,183 +1013,202 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
>      return 0;
>  }
>  
> -/**
> - * Allocate a new file as a volume. This is either done directly
> - * for raw/sparse files, or by calling qemu-img/qcow-create for
> - * special kinds of files
> - */
> -static int
> -virStorageBackendFileSystemVolBuild(virConnectPtr conn,
> -                                    virStorageVolDefPtr vol)
> -{
> +// XXX: Have these functions all use the same format?
> +static int createRaw(virConnectPtr conn,
> +                     virStorageVolDefPtr vol) {
>      int fd;
>  
> -    if (vol->target.format == VIR_STORAGE_VOL_FILE_RAW) {
> -        if ((fd = open(vol->target.path, O_RDWR | O_CREAT | O_EXCL,
> -                       vol->target.perms.mode)) < 0) {
> -            virReportSystemError(conn, errno,
> -                                 _("cannot create path '%s'"),
> -                                 vol->target.path);
> -            return -1;
> -        }
> +    if ((fd = open(vol->target.path, O_RDWR | O_CREAT | O_EXCL,
> +                   vol->target.perms.mode)) < 0) {
> +        virReportSystemError(conn, errno,
> +                             _("cannot create path '%s'"),
> +                             vol->target.path);
> +        return -1;
> +    }
>  
> -        /* Seek to the final size, so the capacity is available upfront
> -         * for progress reporting */
> -        if (ftruncate(fd, vol->capacity) < 0) {
> -            virReportSystemError(conn, errno,
> -                                 _("cannot extend file '%s'"),
> -                                 vol->target.path);
> -            close(fd);
> -            return -1;
> -        }
> +    /* Seek to the final size, so the capacity is available upfront
> +     * for progress reporting */
> +    if (ftruncate(fd, vol->capacity) < 0) {
> +        virReportSystemError(conn, errno,
> +                             _("cannot extend file '%s'"),
> +                             vol->target.path);
> +        close(fd);
> +        return -1;
> +    }
>  
> -        /* Pre-allocate any data if requested */
> -        /* XXX slooooooooooooooooow on non-extents-based file systems */
> -        /* FIXME: Add in progress bars & bg thread if progress bar requested */
> -        if (vol->allocation) {
> -            if (track_allocation_progress) {
> -                unsigned long long remain = vol->allocation;
> -
> -                while (remain) {
> -                    /* Allocate in chunks of 512MiB: big-enough chunk
> -                     * size and takes approx. 9s on ext3. A progress
> -                     * update every 9s is a fair-enough trade-off
> -                     */
> -                    unsigned long long bytes = 512 * 1024 * 1024;
> -                    int r;
> -
> -                    if (bytes > remain)
> -                        bytes = remain;
> -                    if ((r = safezero(fd, 0, vol->allocation - remain,
> -                                      bytes)) != 0) {
> -                        virReportSystemError(conn, r,
> -                                             _("cannot fill file '%s'"),
> -                                             vol->target.path);
> -                        close(fd);
> -                        return -1;
> -                    }
> -                    remain -= bytes;
> -                }
> -            } else { /* No progress bars to be shown */
> +    /* Pre-allocate any data if requested */
> +    /* XXX slooooooooooooooooow on non-extents-based file systems */
> +    if (vol->allocation) {
> +        if (track_allocation_progress) {
> +            unsigned long long remain = vol->allocation;
> +
> +            while (remain) {
> +                /* Allocate in chunks of 512MiB: big-enough chunk
> +                 * size and takes approx. 9s on ext3. A progress
> +                 * update every 9s is a fair-enough trade-off
> +                 */
> +                unsigned long long bytes = 512 * 1024 * 1024;
>                  int r;
>  
> -                if ((r = safezero(fd, 0, 0, vol->allocation)) != 0) {
> +                if (bytes > remain)
> +                    bytes = remain;
> +                if ((r = safezero(fd, 0, vol->allocation - remain,
> +                                  bytes)) != 0) {
>                      virReportSystemError(conn, r,
>                                           _("cannot fill file '%s'"),
>                                           vol->target.path);
>                      close(fd);
>                      return -1;
>                  }
> +                remain -= bytes;
> +            }
> +        } else { /* No progress bars to be shown */
> +            int r;
> +
> +            if ((r = safezero(fd, 0, 0, vol->allocation)) != 0) {
> +                virReportSystemError(conn, r,
> +                                     _("cannot fill file '%s'"),
> +                                     vol->target.path);
> +                close(fd);
> +                return -1;
>              }
>          }
> +    }
>  
> -    } else if (vol->target.format == VIR_STORAGE_VOL_FILE_DIR) {
> -        if (mkdir(vol->target.path, vol->target.perms.mode) < 0) {
> -            virReportSystemError(conn, errno,
> -                                 _("cannot create path '%s'"),
> -                                 vol->target.path);
> -            return -1;
> -        }
> +    if (close(fd) < 0) {
> +        virReportSystemError(conn, errno,
> +                             _("cannot close file '%s'"),
> +                             vol->target.path);
> +        return -1;
> +    }
>  
> -        if ((fd = open(vol->target.path, O_RDWR)) < 0) {
> -            virReportSystemError(conn, errno,
> -                                 _("cannot read path '%s'"),
> -                                 vol->target.path);
> -            return -1;
> -        }
> -    } else {
> -#if HAVE_QEMU_IMG
> -        const char *type = virStorageVolFormatFileSystemTypeToString(vol->target.format);
> -        const char *backingType = vol->backingStore.path ?
> -            virStorageVolFormatFileSystemTypeToString(vol->backingStore.format) : NULL;
> -        char size[100];
> -        const char **imgargv;
> -        const char *imgargvnormal[] = {
> -            QEMU_IMG, "create", "-f", type, vol->target.path, size, NULL,
> -        };
> -        /* XXX including "backingType" here too, once QEMU accepts
> -         * the patches to specify it. It'll probably be -F backingType */
> -        const char *imgargvbacking[] = {
> -            QEMU_IMG, "create", "-f", type, "-b", vol->backingStore.path, vol->target.path, size, NULL,
> -        };
> -
> -        if (type == NULL) {
> -            virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
> -                                  _("unknown storage vol type %d"),
> -                                  vol->target.format);
> -            return -1;
> -        }
> -        if (vol->backingStore.path == NULL) {
> -            imgargv = imgargvnormal;
> -        } else {
> -            if (backingType == NULL) {
> -                virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
> -                                      _("unknown storage vol backing store type %d"),
> -                                      vol->backingStore.format);
> -                return -1;
> -            }
> -            if (access(vol->backingStore.path, R_OK) != 0) {
> -                virReportSystemError(conn, errno,
> -                                     _("inaccessible backing store volume %s"),
> -                                     vol->backingStore.path);
> -                return -1;
> -            }
> +    return 0;
> +}
>  
> -            imgargv = imgargvbacking;
> -        }
> +static int createFileDir(virConnectPtr conn,
> +                         virStorageVolDefPtr vol) {
> +    if (mkdir(vol->target.path, vol->target.perms.mode) < 0) {
> +        virReportSystemError(conn, errno,
> +                             _("cannot create path '%s'"),
> +                             vol->target.path);
> +        return -1;
> +    }
>  
> -        /* Size in KB */
> -        snprintf(size, sizeof(size), "%llu", vol->capacity/1024);
> +    return 0;
> +}
>  
> -        if (virRun(conn, imgargv, NULL) < 0) {
> -            return -1;
> -        }
> +#if HAVE_QEMU_IMG
> +static int createQemuImg(virConnectPtr conn,
> +                         virStorageVolDefPtr vol) {
> +    const char *type = virStorageVolFormatFileSystemTypeToString(vol->target.format);
> +    const char *backingType = vol->backingStore.path ?
> +        virStorageVolFormatFileSystemTypeToString(vol->backingStore.format) : NULL;
> +    char size[100];
> +    const char **imgargv;
> +    const char *imgargvnormal[] = {
> +        QEMU_IMG, "create", "-f", type, vol->target.path, size, NULL,
> +    };
> +    /* XXX including "backingType" here too, once QEMU accepts
> +     * the patches to specify it. It'll probably be -F backingType */
> +    const char *imgargvbacking[] = {
> +        QEMU_IMG, "create", "-f", type, "-b", vol->backingStore.path, vol->target.path, size, NULL,
> +    };
>  
> -        if ((fd = open(vol->target.path, O_RDONLY)) < 0) {
> -            virReportSystemError(conn, errno,
> -                                 _("cannot read path '%s'"),
> -                                 vol->target.path);
> -            return -1;
> -        }
> -#elif HAVE_QCOW_CREATE
> -        /*
> -         * Xen removed the fully-functional qemu-img, and replaced it
> -         * with a partially functional qcow-create. Go figure ??!?
> -         */
> -        char size[100];
> -        const char *imgargv[4];
> -
> -        if (vol->target.format != VIR_STORAGE_VOL_FILE_QCOW2) {
> +    if (type == NULL) {
> +        virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                              _("unknown storage vol type %d"),
> +                              vol->target.format);
> +        return -1;
> +    }
> +    if (vol->backingStore.path == NULL) {
> +        imgargv = imgargvnormal;
> +    } else {
> +        if (backingType == NULL) {
>              virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
> -                                  _("unsupported storage vol type %d"),
> -                                  vol->target.format);
> +                                  _("unknown storage vol backing store type %d"),
> +                                  vol->backingStore.format);
>              return -1;
>          }
> -        if (vol->backingStore.path != NULL) {
> -            virStorageReportError(conn, VIR_ERR_NO_SUPPORT,
> -                                  _("copy-on-write image not supported with "
> -                                    "qcow-create"));
> +        if (access(vol->backingStore.path, R_OK) != 0) {
> +            virReportSystemError(conn, errno,
> +                                 _("inaccessible backing store volume %s"),
> +                                 vol->backingStore.path);
>              return -1;
>          }
>  
> -        /* Size in MB - yes different units to qemu-img :-( */
> -        snprintf(size, sizeof(size), "%llu", vol->capacity/1024/1024);
> +        imgargv = imgargvbacking;
> +    }
>  
> -        imgargv[0] = QCOW_CREATE;
> -        imgargv[1] = size;
> -        imgargv[2] = vol->target.path;
> -        imgargv[3] = NULL;
> +    /* Size in KB */
> +    snprintf(size, sizeof(size), "%llu", vol->capacity/1024);
>  
> -        if (virRun(conn, imgargv, NULL) < 0) {
> -            return -1;
> -        }
> +    if (virRun(conn, imgargv, NULL) < 0) {
> +        return -1;
> +    }
>  
> -        if ((fd = open(vol->target.path, O_RDONLY)) < 0) {
> -            virReportSystemError(conn, errno,
> -                                 _("cannot read path '%s'"),
> -                                 vol->target.path);
> -            return -1;
> -        }
> +    return 0;
> +}
> +
> +#elif HAVE_QCOW_CREATE
> +/*
> + * Xen removed the fully-functional qemu-img, and replaced it
> + * with a partially functional qcow-create. Go figure ??!?
> + */
> +static int createQemuCreate(virConnectPtr conn,
> +                            virStorageVolDefPtr vol) {
> +    char size[100];
> +    const char *imgargv[4];
> +
> +    if (vol->target.format != VIR_STORAGE_VOL_FILE_QCOW2) {
> +        virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                              _("unsupported storage vol type %d"),
> +                              vol->target.format);
> +        return -1;
> +    }
> +    if (vol->backingStore.path != NULL) {
> +        virStorageReportError(conn, VIR_ERR_NO_SUPPORT,
> +                              _("copy-on-write image not supported with "
> +                              "qcow-create"));
> +        return -1;
> +    }
> +
> +    /* Size in MB - yes different units to qemu-img :-( */
> +    snprintf(size, sizeof(size), "%llu", vol->capacity/1024/1024);
> +
> +    imgargv[0] = QCOW_CREATE;
> +    imgargv[1] = size;
> +    imgargv[2] = vol->target.path;
> +    imgargv[3] = NULL;
> +
> +    if (virRun(conn, imgargv, NULL) < 0) {
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +#endif /* HAVE_QEMU_IMG, elif HAVE_QCOW_CREATE */
> +
> +/**
> + * Allocate a new file as a volume. This is either done directly
> + * for raw/sparse files, or by calling qemu-img/qcow-create for
> + * special kinds of files
> + */
> +static int
> +virStorageBackendFileSystemVolBuild(virConnectPtr conn,
> +                                    virStorageVolDefPtr vol)
> +{
> +    int fd;
> +    createFile create_func;
> +
> +    if (vol->target.format == VIR_STORAGE_VOL_FILE_RAW) {
> +        create_func = createRaw;
> +    } else if (vol->target.format == VIR_STORAGE_VOL_FILE_DIR) {
> +        create_func = createFileDir;
> +    } else {
> +#if HAVE_QEMU_IMG
> +        create_func = createQemuImg;
> +#elif HAVE_QCOW_CREATE
> +        create_func = createQemuCreate;
>  #else
>          virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
>                                "%s", _("creation of non-raw images "
> @@ -1196,6 +1217,16 @@ virStorageBackendFileSystemVolBuild(virConnectPtr conn,
>  #endif
>      }
>  
> +    if (create_func(conn, vol) < 0)
> +        return -1;
> +
> +    if ((fd = open(vol->target.path, O_RDONLY)) < 0) {
> +        virReportSystemError(conn, errno,
> +                             _("cannot read path '%s'"),
> +                             vol->target.path);
> +        return -1;
> +    }
> +
>      /* We can only chown/grp if root */
>      if (getuid() == 0) {
>          if (fchown(fd, vol->target.perms.uid, vol->target.perms.gid) < 0) {
> -- 
> 1.6.0.6
> 
> --
> Libvir-list mailing list
> Libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list

-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.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]