[libvirt] [PATCH v2 05/12] getstats: rearrange blockinfo gathering

[Peter Krempa]

On 12/16/14 09:04, Eric Blake wrote:
> We want to avoid read()ing a file while qemu is running.  We still
> have to open() block devices to determine their physical size, but
> that is safer.  This patch rearranges code to make it easier to
> skip the metadata collection when possible.
> 
> * src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Check for empty
> disk up front.  Perform stat first.  Place metadata reading next
> to use.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/qemu/qemu_driver.c | 61 ++++++++++++++++++++++++--------------------------
>  1 file changed, 29 insertions(+), 32 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 7aaee96..f87c44b 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c

> @@ -11066,10 +11072,12 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
>       * change.
>       */
>      if (virStorageSourceIsLocalStorage(disk->src)) {
> -        if (!disk->src->path) {
> -            virReportError(VIR_ERR_INVALID_ARG,
> -                           _("disk '%s' does not currently have a source assigned"),
> -                           path);
> +        /* Yes, this is a mild TOCTTOU race, but if someone is
> +         * changing files in the background behind libvirt's back,
> +         * they deserve bogus information.  */
> +        if (stat(disk->src->path, &sb) < 0) {
> +            virReportSystemError(errno,
> +                                 _("cannot stat file '%s'"), disk->src->path);
>              goto endjob;
>          }

Um this will cause problems on NFS. The code after that that you are
moving uses the FD to do the stat() call. The fd is opened by the helper
that opens the file with correct perms.

> 
> @@ -11077,12 +11085,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
>                                 NULL, NULL)) == -1)
>              goto endjob;
> 
> -        if (fstat(fd, &sb) < 0) {
> -            virReportSystemError(errno,
> -                                 _("cannot stat file '%s'"), disk->src->path);
> -            goto endjob;
> -        }
> -
>          if ((len = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, &buf)) < 0) {
>              virReportSystemError(errno, _("cannot read header '%s'"),
>                                   disk->src->path);


Otherwise looks okay.

Peter

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141216/5b0f1082/attachment-0001.sig>