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

Re: [libvirt] [PATCH v3 08/12] qemu_domain: Introduce qemuDomainGetDiskBlockInfo



On 02/06/2013 08:52 AM, Michal Privoznik wrote:
> This is just digging out important implementation from qemu
> driver's qemuDomainGetDiskBlockInfo() API as this functionality
> is going to be required in the next patch.
> ---
>  src/qemu/qemu_domain.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_domain.h |   4 ++
>  src/qemu/qemu_driver.c | 124 +++--------------------------------------------
>  3 files changed, 138 insertions(+), 117 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 5bf0ab0..d7d7163 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -40,6 +40,9 @@
>  
>  #include <sys/time.h>
>  #include <fcntl.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
>  
>  #include <libxml/xpathInternals.h>
>  
> @@ -2073,3 +2076,127 @@ cleanup:
>      virObjectUnref(cfg);
>      return ret;
>  }
> +
> +int
> +qemuDomainGetDiskBlockInfo(virQEMUDriverPtr driver,
> +                           virDomainObjPtr vm,
> +                           virDomainDiskDefPtr disk,
> +                           virDomainBlockInfoPtr info)
> +{
> +
> +    int ret = -1;
> +    virStorageFileMetadata *meta = NULL;
> +    virQEMUDriverConfigPtr cfg = NULL;
> +    int format;
> +    struct stat sb;
> +    int fd = -1;
> +    off_t end;
> +    const char *path;
> +
> +    if (!disk->src) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("disk %s does not currently have a source assigned"),
> +                       disk->dst);
> +        goto cleanup;
> +    }
> +    path = disk->src;
> +    cfg = virQEMUDriverGetConfig(driver);
> +
> +    /* The path is correct, now try to open it and get its size. */
> +    if ((fd = open(path, O_RDONLY)) < 0) {
> +        virReportSystemError(errno, _("failed to open path '%s'"), path);
> +        goto cleanup;
> +    }
> +
> +    /* Probe for magic formats */
> +    if (disk->format) {
> +        format = disk->format;
> +    } else {
> +        if (cfg->allowDiskFormatProbing) {
> +            if ((format = virStorageFileProbeFormat(path,
> +                                                    cfg->user,
> +                                                    cfg->group)) < 0)
> +                goto cleanup;
> +        } else {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("no disk format for %s and probing is disabled"),
> +                           path);
> +            goto cleanup;
> +        }
> +    }
> +
> +    if (!(meta = virStorageFileGetMetadataFromFD(path, fd, format)))
> +        goto cleanup;
> +
> +    /* Get info for normal formats */
> +    if (fstat(fd, &sb) < 0) {
> +        virReportSystemError(errno, _("cannot stat file '%s'"), path);
> +        goto cleanup;
> +    }
> +
> +    if (S_ISREG(sb.st_mode)) {
> +#ifndef WIN32
> +        info->physical = (unsigned long long)sb.st_blocks *
> +            (unsigned long long)DEV_BSIZE;
> +#else
> +        info->physical = sb.st_size;
> +#endif
> +        /* Regular files may be sparse, so logical size (capacity) is not same
> +         * as actual physical above
> +         */
> +        info->capacity = sb.st_size;
> +    } else {
> +        /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should
> +         * be 64 bits on all platforms.
> +         */
> +        end = lseek(fd, 0, SEEK_END);
> +        if (end == (off_t)-1) {
> +            virReportSystemError(errno, _("failed to seek to end of %s"), path);
> +            goto cleanup;
> +        }
> +        info->physical = end;
> +        info->capacity = end;
> +    }
> +
> +    /* If the file we probed has a capacity set, then override
> +     * what we calculated from file/block extents */
> +    if (meta->capacity)
> +        info->capacity = meta->capacity;
> +
> +    /* Set default value .. */
> +    info->allocation = info->physical;
> +
> +    /* ..but if guest is running & not using raw
> +       disk format and on a block device, then query
> +       highest allocated extent from QEMU */
> +    if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK &&
> +        format != VIR_STORAGE_FILE_RAW &&
> +        S_ISBLK(sb.st_mode) &&
> +        virDomainObjIsActive(vm)) {
> +        qemuDomainObjPrivatePtr priv = vm->privateData;
> +
> +        if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
> +            goto cleanup;
> +
> +        if (virDomainObjIsActive(vm)) {
> +            qemuDomainObjEnterMonitor(driver, vm);
> +            ret = qemuMonitorGetBlockExtent(priv->mon,
> +                                            disk->info.alias,
> +                                            &info->allocation);
> +            qemuDomainObjExitMonitor(driver, vm);
> +        } else {
> +            ret = 0;
> +        }
> +
> +        if (qemuDomainObjEndJob(driver, vm) == 0)
> +            vm = NULL;

I don't believe this will have the same effect as the prior code. The
'vm' is not passed by reference, thus setting it to NULL here just does
so for this routine.  When you return to the caller vm is what it was at
call time.  When I saw this here without seeing you had lifted code
below I was wondering why this was done since it's essentially a NOP.

> +    } else {
> +        ret = 0;
> +    }
> +
> +cleanup:
> +    VIR_FORCE_CLOSE(fd);
> +    virStorageFileFreeMetadata(meta);
> +    virObjectUnref(cfg);
> +    return ret;
> +}
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index fb58877..e8555d3 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -359,5 +359,9 @@ void qemuDomainCleanupRemove(virDomainObjPtr vm,
>                               qemuDomainCleanupCallback cb);
>  void qemuDomainCleanupRun(virQEMUDriverPtr driver,
>                            virDomainObjPtr vm);
> +int qemuDomainGetDiskBlockInfo(virQEMUDriverPtr driver,
> +                               virDomainObjPtr vm,
> +                               virDomainDiskDefPtr disk,
> +                               virDomainBlockInfoPtr info);
>  
>  #endif /* __QEMU_DOMAIN_H__ */
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 979a027..c3d0dc3 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -9212,29 +9212,23 @@ cleanup:
>  }
>  
>  
> -static int qemuDomainGetBlockInfo(virDomainPtr dom,
> -                                  const char *path,
> -                                  virDomainBlockInfoPtr info,
> -                                  unsigned int flags) {
> +static int
> +qemuDomainGetBlockInfo(virDomainPtr dom,
> +                       const char *path,
> +                       virDomainBlockInfoPtr info,
> +                       unsigned int flags)
> +{
>      virQEMUDriverPtr driver = dom->conn->privateData;
>      virDomainObjPtr vm;
>      int ret = -1;
> -    int fd = -1;
> -    off_t end;
> -    virStorageFileMetadata *meta = NULL;
>      virDomainDiskDefPtr disk = NULL;
> -    struct stat sb;
>      int i;
> -    int format;
> -    virQEMUDriverConfigPtr cfg = NULL;
>  
>      virCheckFlags(0, -1);
>  
>      if (!(vm = qemuDomObjFromDomain(dom)))
>          goto cleanup;
>  
> -    cfg = virQEMUDriverGetConfig(driver);
> -
>      if (!path || path[0] == '\0') {
>          virReportError(VIR_ERR_INVALID_ARG,
>                         "%s", _("NULL or empty path"));
> @@ -9248,116 +9242,12 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom,
>          goto cleanup;
>      }
>      disk = vm->def->disks[i];
> -    if (!disk->src) {
> -        virReportError(VIR_ERR_INVALID_ARG,
> -                       _("disk %s does not currently have a source assigned"),
> -                       path);
> -        goto cleanup;
> -    }
> -    path = disk->src;
> -
> -    /* The path is correct, now try to open it and get its size. */
> -    fd = open(path, O_RDONLY);
> -    if (fd == -1) {
> -        virReportSystemError(errno,
> -                             _("failed to open path '%s'"), path);
> -        goto cleanup;
> -    }
> -
> -    /* Probe for magic formats */
> -    if (disk->format) {
> -        format = disk->format;
> -    } else {
> -        if (cfg->allowDiskFormatProbing) {
> -            if ((format = virStorageFileProbeFormat(disk->src,
> -                                                    cfg->user,
> -                                                    cfg->group)) < 0)
> -                goto cleanup;
> -        } else {
> -            virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("no disk format for %s and probing is disabled"),
> -                           disk->src);
> -            goto cleanup;
> -        }
> -    }
> -
> -    if (!(meta = virStorageFileGetMetadataFromFD(path, fd, format)))
> -        goto cleanup;
> -
> -    /* Get info for normal formats */
> -    if (fstat(fd, &sb) < 0) {
> -        virReportSystemError(errno,
> -                             _("cannot stat file '%s'"), path);
> -        goto cleanup;
> -    }
>  
> -    if (S_ISREG(sb.st_mode)) {
> -#ifndef WIN32
> -        info->physical = (unsigned long long)sb.st_blocks *
> -            (unsigned long long)DEV_BSIZE;
> -#else
> -        info->physical = sb.st_size;
> -#endif
> -        /* Regular files may be sparse, so logical size (capacity) is not same
> -         * as actual physical above
> -         */
> -        info->capacity = sb.st_size;
> -    } else {
> -        /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should
> -         * be 64 bits on all platforms.
> -         */
> -        end = lseek(fd, 0, SEEK_END);
> -        if (end == (off_t)-1) {
> -            virReportSystemError(errno,
> -                                 _("failed to seek to end of %s"), path);
> -            goto cleanup;
> -        }
> -        info->physical = end;
> -        info->capacity = end;
> -    }
> -
> -    /* If the file we probed has a capacity set, then override
> -     * what we calculated from file/block extents */
> -    if (meta->capacity)
> -        info->capacity = meta->capacity;
> -
> -    /* Set default value .. */
> -    info->allocation = info->physical;
> -
> -    /* ..but if guest is running & not using raw
> -       disk format and on a block device, then query
> -       highest allocated extent from QEMU */
> -    if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK &&
> -        format != VIR_STORAGE_FILE_RAW &&
> -        S_ISBLK(sb.st_mode) &&
> -        virDomainObjIsActive(vm)) {
> -        qemuDomainObjPrivatePtr priv = vm->privateData;
> -
> -        if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
> -            goto cleanup;
> -
> -        if (virDomainObjIsActive(vm)) {
> -            qemuDomainObjEnterMonitor(driver, vm);
> -            ret = qemuMonitorGetBlockExtent(priv->mon,
> -                                            disk->info.alias,
> -                                            &info->allocation);
> -            qemuDomainObjExitMonitor(driver, vm);
> -        } else {
> -            ret = 0;
> -        }
> -
> -        if (qemuDomainObjEndJob(driver, vm) == 0)
> -            vm = NULL;
> -    } else {
> -        ret = 0;
> -    }
> +    ret = qemuDomainGetDiskBlockInfo(driver, vm, disk, info);
>  
>  cleanup:
> -    virStorageFileFreeMetadata(meta);
> -    VIR_FORCE_CLOSE(fd);
>      if (vm)
>          virObjectUnlock(vm);

See note above in the qemuDomainGetDiskBlockInfo() routine

> -    virObjectUnref(cfg);
>      return ret;
>  }
>  
> 


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