[PATCH 32/36] qemu: Prepare storage backing chain traversal code for FD passed images

[Pavel Hrdina]

On Thu, Jan 05, 2023 at 05:30:21PM +0100, Peter Krempa wrote:
> We assume that FD passed images already exist so all existance checks
> are skipped.
> 
> For the case that a FD-passed image is passed without a terminated
> backing chain (thus forcing us to detect) we attempt to read the header
> from the FD.
> 
> Signed-off-by: Peter Krempa <pkrempa at redhat.com>
> ---
>  src/qemu/qemu_domain.c            | 23 ++++++++++++++---------
>  src/storage_file/storage_source.c | 14 ++++++++++++++
>  2 files changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 7dc4ef4ddb..38883a57d8 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7679,16 +7679,20 @@ qemuDomainDetermineDiskChain(virQEMUDriver *driver,
>          disksrc->format > VIR_STORAGE_FILE_NONE &&
>          disksrc->format < VIR_STORAGE_FILE_BACKING) {
> 
> +        /* terminate the chain for such images as the code below would do */
> +        if (!disksrc->backingStore)
> +            disksrc->backingStore = virStorageSourceNew();
> +
> +        /* we assume that FD-passed disks always exist */
> +        if (virStorageSourceIsFD(disksrc))
> +            return 0;
> +
>          if (!virFileExists(disksrc->path)) {
>              virStorageSourceReportBrokenChain(errno, disksrc, disksrc);
> 
>              return -1;
>          }
> 
> -        /* terminate the chain for such images as the code below would do */
> -        if (!disksrc->backingStore)
> -            disksrc->backingStore = virStorageSourceNew();
> -
>          /* host cdrom requires special treatment in qemu, so we need to check
>           * whether a block device is a cdrom */
>          if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM &&
> @@ -7700,12 +7704,14 @@ qemuDomainDetermineDiskChain(virQEMUDriver *driver,
>          return 0;
>      }
> 
> -    src = disksrc;
>      /* skip to the end of the chain if there is any */
> -    while (virStorageSourceHasBacking(src)) {
> -        int rv = virStorageSourceSupportsAccess(src);
> +    for (src = disksrc; virStorageSourceHasBacking(src); src = src->backingStore) {
> +        int rv;
> +
> +        if (virStorageSourceIsFD(src))
> +            continue;
> 
> -        if (rv < 0)
> +        if ((rv = virStorageSourceSupportsAccess(src)) < 0)
>              return -1;
> 
>          if (rv > 0) {
> @@ -7720,7 +7726,6 @@ qemuDomainDetermineDiskChain(virQEMUDriver *driver,
> 
>              virStorageSourceDeinit(src);
>          }
> -        src = src->backingStore;
>      }
> 
>      /* We skipped to the end of the chain. Skip detection if there's the
> diff --git a/src/storage_file/storage_source.c b/src/storage_file/storage_source.c
> index ab0cdf2b12..00b28f73a6 100644
> --- a/src/storage_file/storage_source.c
> +++ b/src/storage_file/storage_source.c
> @@ -1264,6 +1264,20 @@ virStorageSourceGetMetadataRecurseReadHeader(virStorageSource *src,
>      int ret = -1;
>      ssize_t len;
> 
> +    if (virStorageSourceIsFD(src)) {
> +        if (!src->fdtuple) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("fd passed image source not initialized"));
> +            return -1;
> +        }
> +
> +        return virFileReadHeaderFD(src->fdtuple->fds[0], VIR_STORAGE_MAX_HEADER,
> +                                   buf);
> +    }

This change makes the compiler complain with the following error:

../src/storage_file/storage_source.c: In function ‘virStorageSourceGetMetadataRecurse’:
../src/storage_file/storage_source.c:1343:9: error: ‘headerLen’ may be used uninitialized [-Werror=maybe-uninitialized]
 1343 |     if (virStorageFileProbeGetMetadata(src, buf, headerLen) < 0)
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/storage_file/storage_source.c:1311:12: note: ‘headerLen’ was declared here
 1311 |     size_t headerLen;

The issue is that at the end of this function we set `*headerLen = len;`
but it doesn't happen with FD storage source, instead we return the len
and the return value of virStorageSourceGetMetadataRecurseReadHeader()
is only used in if condition.

> +
> +    if (src->fdgroup)
> +        return 0;

This seems like no-op as virStorageSourceIsFD() does the same.

Pavel

>      if (virStorageSourceInitAs(src, uid, gid) < 0)
>          return -1;
> 
> -- 
> 2.38.1
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20230106/57f48305/attachment.sig>