[libvirt] [PATCH 4/6] Implement virDomainGetBlockInfo in QEMU driver

[Jim Meyering]

Daniel P. Berrange wrote:
> * src/qemu/qemu_driver.c: Implementation of virDomainGetBlockInfo
> * src/util/storage_file.h: Add DEV_BSIZE
> * src/storage/storage_backend.c: Remove DEV_BSIZE
...

Hi Dan,
This whole series looks fine.

> +static int qemuDomainGetBlockInfo(virDomainPtr dom,

It's slightly better to format with both the function name and the "{"
in the first column.  That makes it easier for certain function-locating
tools (and even humans who are used to that) to find the beginning of
a function.  It also saves horizontal space, which can be important with
some of these very long function names.

static int
qemuDomainGetBlockInfo(virDomainPtr dom,
                       ...
{

> +                                  const char *path,
> +                                  virDomainBlockInfoPtr info,
> +                                  unsigned int flags) {
> +    struct qemud_driver *driver = dom->conn->privateData;
> +    virDomainObjPtr vm;
> +    }
...
> +    /* Probe for magic formats */
> +    memset(&meta, 0, sizeof(meta));
> +    if (virStorageFileGetMetadataFromFD(path, fd, &meta) < 0)
> +        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 __MINGW32__
> +        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 {

It might be worthwhile to change this to handle S_ISBLK
specifically:

       } else if (S_ISBLK(sb.st_mode)) {

and to add a final "else" so that the code diagnoses
a non-regular, non-BLOCK file.  Then, we can give a better
diagnostic about the file being of a non-seekable type.

On the other hand, its's not very likely we'll encounter such a
file here, so no big deal either way.

> +        /* 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;
> +
> +    /* XXX allocation will need to be pulled from QEMU for
> +     * the qcow inside LVM case */
> +    info->allocation = info->physical;
> +
> +    ret = 0;
> +
> +cleanup:
> +    if (fd != -1)
> +        close(fd);
> +    if (vm)
> +        virDomainObjUnlock(vm);
> +    return ret;
> +}