On Wed, Nov 20, 2024 at 18:48:42 +0300, Nikolai Barybin via Devel wrote:
> Signed-off-by: Nikolai Barybin <nikolai.barybin@xxxxxxxxxxxxx>
> ---
> src/storage_file/storage_source.c | 39 +++++++++++++++++++++++++++++++
> src/storage_file/storage_source.h | 4 ++++
> 2 files changed, 43 insertions(+)
>
> diff --git a/src/storage_file/storage_source.c b/src/storage_file/storage_source.c
> index 2cfe3bc325..b9d2d71aea 100644
> --- a/src/storage_file/storage_source.c
> +++ b/src/storage_file/storage_source.c
> @@ -543,6 +543,33 @@ virStorageSourceNewFromBacking(virStorageSource *parent,
> }
>
>
> +/**
> + * virStorageSourceNewFromDataFile:
> + * @parent: storage source parent
> + * @dataFileSrc: returned data file source definition
> + *
> + * Creates a storage source which describes the data file image of @parent.
> + * Returned storage source format is VIR_STORAGE_FILE_RAW, and, unlike
> + * backing storage creation, readonly flag is copied from @parent.
> + *
> + * Return codes are the same as in virStorageSourceNewFromChild.
> + */
> +int
This function is never used outside of this module thus should not be
exported.
> +virStorageSourceNewFromDataFile(virStorageSource *parent,
> + virStorageSource **dataFileSrc)
> +{
> + if (virStorageSourceNewFromChild(parent,
> + parent->dataFileRaw,
> + dataFileSrc) < 0)
> + return -1;
This strips the possibility to return '1' which you've explicitly
documented above.
Thinking about it a bit; the logic which is for backing store regarding
the skipping of detection if libvirt can't parse it is a stop-gap
measure for pre-existing configs when the blockdev code was being
introduced. It meant to simply skip the backing store definition via
-blockdev in case libvirt can't do that and offloaded it to qemu.
For data files I don't think we need this kind of logic, and can fail
instead.
> +
> + (*dataFileSrc)->format = VIR_STORAGE_FILE_RAW;
> + (*dataFileSrc)->readonly = parent->readonly;
> +
> + return 0;
> +}
> +
> +
> /**
> * @src: disk source definition structure
> * @fd: file descriptor
> @@ -1391,6 +1418,18 @@ virStorageSourceGetMetadataRecurse(virStorageSource *src,
> }
> }
>
> + if (src->dataFileRaw) {
> + g_autoptr(virStorageSource) dataFileStore = NULL;
> + if ((rv = virStorageSourceNewFromDataFile(src, &dataFileStore)) < 0)
> + return -1;
> +
> + /* the data file would not be usable for VM usage */
> + if (rv == 1)
> + return 0;
Apart from what I wrote above, this would be wrong here as this would
also skip the backing store parsing below. Now with the change to report
error if 'data-store' can't be fully processed by libvirt the location
and logich here will be correct, but the above return statement will be
removed.
> +
I'll be changing this to:
diff --git a/src/storage_file/storage_source.c b/src/storage_file/storage_source.c
index 2cfe3bc325..df73f44699 100644
--- a/src/storage_file/storage_source.c
+++ b/src/storage_file/storage_source.c
@@ -543,6 +543,39 @@ virStorageSourceNewFromBacking(virStorageSource *parent,
}
+/**
+ * virStorageSourceNewFromDataFile:
+ * @parent: storage source parent
+ *
+ * Creates a storage source which describes the data file image of @parent.
+ * Returned storage source format is VIR_STORAGE_FILE_RAW, and, unlike
+ * backing storage creation, readonly flag is copied from @parent.
+ */
+static virStorageSource *
+virStorageSourceNewFromDataFile(virStorageSource *parent)
+{
+ g_autoptr(virStorageSource) dataFile = NULL;
+ int rc;
+
+ if ((rc = virStorageSourceNewFromChild(parent,
+ parent->dataFileRaw,
+ &dataFile)) < 0)
+ return NULL;
+
+ if (rc == 1) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("can't use data file definition '%1$s'"),
+ parent->dataFileRaw);
+ return NULL;
+ }
+
+ dataFile->format = VIR_STORAGE_FILE_RAW;
+ dataFile->readonly = parent->readonly;
+
+ return g_steal_pointer(&dataFile);
+}
+
+
/**
* @src: disk source definition structure
* @fd: file descriptor
@@ -1391,6 +1424,11 @@ virStorageSourceGetMetadataRecurse(virStorageSource *src,
}
}
+ if (src->dataFileRaw) {
+ if (!(src->dataFileStore = virStorageSourceNewFromDataFile(src)))
+ return -1;
+ }
+
if (src->backingStoreRaw) {
if ((rv = virStorageSourceNewFromBacking(src, &backingStore)) < 0)
return -1;
