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@xxxxxxxxxx>
> ---
> 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
>
Attachment:
signature.asc
Description: PGP signature
