[PATCH] storage: virStorageVolDefParse and storageVolCreateXML flags fix
When the new storage was created using virsh with --validate option following errors occurred: # virsh vol-create default --file vol-def.xml --validate error: Failed to create vol from vol-def.xml error: unsupported flags (0x4) in function virStorageVolDefParseXML and after virStorageVolDefParse fix: # virsh vol-create default --file vol-def.xml --validate error: Failed to create vol from vol-def.xml error: unsupported flags (0x4) in function storageBackendCreateQemuImg Clear the VIR_STORAGE_VOL_CREATE_VALIDATE flag before virStorageVolDefParseXML and backend->buildVol (traces down to storageBackendCreateQemuImg) calls, as the XML schema validation is already complete within previous steps and there is no validation later. Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- NEWS.rst | 5 +++++ src/conf/storage_conf.c | 2 ++ src/storage/storage_driver.c | 4 +++- 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/NEWS.rst b/NEWS.rst index e2dc4e508b..dd345bad7b 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -28,6 +28,11 @@ v11.3.0 (unreleased) * **Bug fixes** + * storage: Fix new volume creation + + No more errors occur when new storage volume is being created + using ``vol-create`` with ``--validate`` option. + v11.2.0 (2025-04-01) ==================== diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 68842004b7..f6d804bb39 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1409,6 +1409,8 @@ virStorageVolDefParse(virStoragePoolDef *pool, "volume", &ctxt, "storagevol.rng", validate))) return NULL; + flags &= ~(VIR_STORAGE_VOL_CREATE_VALIDATE); + return virStorageVolDefParseXML(pool, ctxt, flags); } diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 86c03762d2..2f5a26bbef 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1877,6 +1877,7 @@ storageVolCreateXML(virStoragePoolPtr pool, virStorageVolPtr vol = NULL, newvol = NULL; g_autoptr(virStorageVolDef) voldef = NULL; unsigned int parseFlags = VIR_VOL_XML_PARSE_OPT_CAPACITY; + unsigned int buildFlags = flags; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA | VIR_STORAGE_VOL_CREATE_VALIDATE, NULL); @@ -1953,8 +1954,8 @@ storageVolCreateXML(virStoragePoolPtr pool, voldef->building = true; virObjectUnlock(obj); - buildret = backend->buildVol(obj, buildvoldef, flags); + buildFlags &= ~(VIR_STORAGE_VOL_CREATE_VALIDATE); + buildret = backend->buildVol(obj, buildvoldef, buildFlags); VIR_FREE(buildvoldef); -- 2.48.1
On Mon, Apr 07, 2025 at 14:25:43 +0200, Kirill Shchetiniuk via Devel wrote:
When the new storage was created using virsh with --validate option following errors occurred:
# virsh vol-create default --file vol-def.xml --validate error: Failed to create vol from vol-def.xml error: unsupported flags (0x4) in function virStorageVolDefParseXML
and after virStorageVolDefParse fix:
# virsh vol-create default --file vol-def.xml --validate error: Failed to create vol from vol-def.xml error: unsupported flags (0x4) in function storageBackendCreateQemuImg
Clear the VIR_STORAGE_VOL_CREATE_VALIDATE flag before virStorageVolDefParseXML and backend->buildVol (traces down to storageBackendCreateQemuImg) calls, as the XML schema validation is already complete within previous steps and there is no validation later.
Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- NEWS.rst | 5 +++++ src/conf/storage_conf.c | 2 ++ src/storage/storage_driver.c | 4 +++- 3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/NEWS.rst b/NEWS.rst index e2dc4e508b..dd345bad7b 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -28,6 +28,11 @@ v11.3.0 (unreleased)
* **Bug fixes**
+ * storage: Fix new volume creation + + No more errors occur when new storage volume is being created + using ``vol-create`` with ``--validate`` option.
Changes to NEWS must be always in a separate patch. Mainly you definitely do not want to have conflicts when backporting patches, where NEWS definitely do not match.
v11.2.0 (2025-04-01) ==================== diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 68842004b7..f6d804bb39 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1409,6 +1409,8 @@ virStorageVolDefParse(virStoragePoolDef *pool, "volume", &ctxt, "storagevol.rng", validate))) return NULL;
+ flags &= ~(VIR_STORAGE_VOL_CREATE_VALIDATE);
This function gets VIR_VOL_XML_PARSE_VALIDATE. In storageVolCreateXML VIR_STORAGE_VOL_CREATE_VALIDATE is converted to VIR_VOL_XML_PARSE_VALIDATE. Where did VIR_STORAGE_VOL_CREATE_VALIDATE leak from? Either way this hunk is incorrect as this flag should not get here and the caller needs to be fixed.
On Mon, Apr 07, 2025 at 14:49:14 +0200, Peter Krempa via Devel wrote:
On Mon, Apr 07, 2025 at 14:25:43 +0200, Kirill Shchetiniuk via Devel wrote:
When the new storage was created using virsh with --validate option following errors occurred:
# virsh vol-create default --file vol-def.xml --validate error: Failed to create vol from vol-def.xml error: unsupported flags (0x4) in function virStorageVolDefParseXML
and after virStorageVolDefParse fix:
# virsh vol-create default --file vol-def.xml --validate error: Failed to create vol from vol-def.xml error: unsupported flags (0x4) in function storageBackendCreateQemuImg
Clear the VIR_STORAGE_VOL_CREATE_VALIDATE flag before virStorageVolDefParseXML and backend->buildVol (traces down to storageBackendCreateQemuImg) calls, as the XML schema validation is already complete within previous steps and there is no validation later.
Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- NEWS.rst | 5 +++++ src/conf/storage_conf.c | 2 ++ src/storage/storage_driver.c | 4 +++- 3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/NEWS.rst b/NEWS.rst index e2dc4e508b..dd345bad7b 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -28,6 +28,11 @@ v11.3.0 (unreleased)
* **Bug fixes**
+ * storage: Fix new volume creation + + No more errors occur when new storage volume is being created + using ``vol-create`` with ``--validate`` option.
Changes to NEWS must be always in a separate patch. Mainly you definitely do not want to have conflicts when backporting patches, where NEWS definitely do not match.
v11.2.0 (2025-04-01) ==================== diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 68842004b7..f6d804bb39 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1409,6 +1409,8 @@ virStorageVolDefParse(virStoragePoolDef *pool, "volume", &ctxt, "storagevol.rng", validate))) return NULL;
+ flags &= ~(VIR_STORAGE_VOL_CREATE_VALIDATE);
This function gets VIR_VOL_XML_PARSE_VALIDATE. In storageVolCreateXML VIR_STORAGE_VOL_CREATE_VALIDATE is converted to VIR_VOL_XML_PARSE_VALIDATE.
Where did VIR_STORAGE_VOL_CREATE_VALIDATE leak from?
Either way this hunk is incorrect as this flag should not get here and the caller needs to be fixed.
If in fact you meant to mask out VIR_VOL_XML_PARSE_VALIDATE here I suggest you rather accept it in the virCheckFlags inside the parser code rather than masking it out.
I've discussed this with Michal Privoznik and we decided it's better to mask the flag rather than adding it to the virCheckFlags, as virStorageVolDefParseXML do not validate anything and we did not want to have useless flag inside it, as it not really obvious why it would be there, when function do not validate anything. What do you think? On Mon, Apr 7, 2025 at 3:11 PM Peter Krempa <pkrempa@redhat.com> wrote:
On Mon, Apr 07, 2025 at 14:49:14 +0200, Peter Krempa via Devel wrote:
On Mon, Apr 07, 2025 at 14:25:43 +0200, Kirill Shchetiniuk via Devel wrote:
When the new storage was created using virsh with --validate option following errors occurred:
# virsh vol-create default --file vol-def.xml --validate error: Failed to create vol from vol-def.xml error: unsupported flags (0x4) in function virStorageVolDefParseXML
and after virStorageVolDefParse fix:
# virsh vol-create default --file vol-def.xml --validate error: Failed to create vol from vol-def.xml error: unsupported flags (0x4) in function storageBackendCreateQemuImg
Clear the VIR_STORAGE_VOL_CREATE_VALIDATE flag before virStorageVolDefParseXML and backend->buildVol (traces down to storageBackendCreateQemuImg) calls, as the XML schema validation is already complete within previous steps and there is no validation later.
Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- NEWS.rst | 5 +++++ src/conf/storage_conf.c | 2 ++ src/storage/storage_driver.c | 4 +++- 3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/NEWS.rst b/NEWS.rst index e2dc4e508b..dd345bad7b 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -28,6 +28,11 @@ v11.3.0 (unreleased)
* **Bug fixes**
+ * storage: Fix new volume creation + + No more errors occur when new storage volume is being created + using ``vol-create`` with ``--validate`` option.
Changes to NEWS must be always in a separate patch. Mainly you definitely do not want to have conflicts when backporting patches, where NEWS definitely do not match.
v11.2.0 (2025-04-01) ==================== diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 68842004b7..f6d804bb39 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1409,6 +1409,8 @@ virStorageVolDefParse(virStoragePoolDef *pool, "volume", &ctxt, "storagevol.rng", validate))) return NULL;
+ flags &= ~(VIR_STORAGE_VOL_CREATE_VALIDATE);
This function gets VIR_VOL_XML_PARSE_VALIDATE. In storageVolCreateXML VIR_STORAGE_VOL_CREATE_VALIDATE is converted to VIR_VOL_XML_PARSE_VALIDATE.
Where did VIR_STORAGE_VOL_CREATE_VALIDATE leak from?
Either way this hunk is incorrect as this flag should not get here and the caller needs to be fixed.
If in fact you meant to mask out VIR_VOL_XML_PARSE_VALIDATE here I suggest you rather accept it in the virCheckFlags inside the parser code rather than masking it out.
On Mon, Apr 07, 2025 at 15:25:58 +0200, Kirill Shchetiniuk wrote: Please do not top-post on technical lists [1].
I've discussed this with Michal Privoznik and we decided it's better to mask the flag rather than adding it to the virCheckFlags, as virStorageVolDefParseXML do not validate anything and we did not want to have useless flag inside it, as it not really obvious why it would be there, when function do not validate anything. What do you think?
I consider the idea of 'virCheckFlags' to be more in terms of "The code below correctly handles $FLAGS." Correctly handling means also doing nothing if nothing needs to be done. In case of other parser functions we usually pass in also the validation flag. Athough those do not have any subsequent virCheckFlags checks. In any case there's nothing bad with having a flag that is not checked that happens regularly.
On Mon, Apr 7, 2025 at 3:11 PM Peter Krempa <pkrempa@redhat.com> wrote:
On Mon, Apr 07, 2025 at 14:49:14 +0200, Peter Krempa via Devel wrote:
On Mon, Apr 07, 2025 at 14:25:43 +0200, Kirill Shchetiniuk via Devel wrote:
When the new storage was created using virsh with --validate option following errors occurred:
# virsh vol-create default --file vol-def.xml --validate error: Failed to create vol from vol-def.xml error: unsupported flags (0x4) in function virStorageVolDefParseXML
and after virStorageVolDefParse fix:
# virsh vol-create default --file vol-def.xml --validate error: Failed to create vol from vol-def.xml error: unsupported flags (0x4) in function storageBackendCreateQemuImg
Clear the VIR_STORAGE_VOL_CREATE_VALIDATE flag before virStorageVolDefParseXML and backend->buildVol (traces down to storageBackendCreateQemuImg) calls, as the XML schema validation is already complete within previous steps and there is no validation later.
Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- NEWS.rst | 5 +++++ src/conf/storage_conf.c | 2 ++ src/storage/storage_driver.c | 4 +++- 3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/NEWS.rst b/NEWS.rst index e2dc4e508b..dd345bad7b 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -28,6 +28,11 @@ v11.3.0 (unreleased)
* **Bug fixes**
+ * storage: Fix new volume creation + + No more errors occur when new storage volume is being created + using ``vol-create`` with ``--validate`` option.
Changes to NEWS must be always in a separate patch. Mainly you definitely do not want to have conflicts when backporting patches, where NEWS definitely do not match.
v11.2.0 (2025-04-01) ==================== diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 68842004b7..f6d804bb39 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1409,6 +1409,8 @@ virStorageVolDefParse(virStoragePoolDef *pool, "volume", &ctxt, "storagevol.rng", validate))) return NULL;
+ flags &= ~(VIR_STORAGE_VOL_CREATE_VALIDATE);
This function gets VIR_VOL_XML_PARSE_VALIDATE. In storageVolCreateXML VIR_STORAGE_VOL_CREATE_VALIDATE is converted to VIR_VOL_XML_PARSE_VALIDATE.
Where did VIR_STORAGE_VOL_CREATE_VALIDATE leak from?
Either way this hunk is incorrect as this flag should not get here and the caller needs to be fixed.
If in fact you meant to mask out VIR_VOL_XML_PARSE_VALIDATE here I suggest you rather accept it in the virCheckFlags inside the parser code rather than masking it out.
a
[1] This is why top posting is so bad (in top posting order): A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in email?
On 4/7/25 15:11, Peter Krempa via Devel wrote:
On Mon, Apr 07, 2025 at 14:49:14 +0200, Peter Krempa via Devel wrote:
On Mon, Apr 07, 2025 at 14:25:43 +0200, Kirill Shchetiniuk via Devel wrote:
When the new storage was created using virsh with --validate option following errors occurred:
# virsh vol-create default --file vol-def.xml --validate error: Failed to create vol from vol-def.xml error: unsupported flags (0x4) in function virStorageVolDefParseXML
and after virStorageVolDefParse fix:
# virsh vol-create default --file vol-def.xml --validate error: Failed to create vol from vol-def.xml error: unsupported flags (0x4) in function storageBackendCreateQemuImg
Clear the VIR_STORAGE_VOL_CREATE_VALIDATE flag before virStorageVolDefParseXML and backend->buildVol (traces down to storageBackendCreateQemuImg) calls, as the XML schema validation is already complete within previous steps and there is no validation later.
Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- NEWS.rst | 5 +++++ src/conf/storage_conf.c | 2 ++ src/storage/storage_driver.c | 4 +++- 3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/NEWS.rst b/NEWS.rst index e2dc4e508b..dd345bad7b 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -28,6 +28,11 @@ v11.3.0 (unreleased)
* **Bug fixes**
+ * storage: Fix new volume creation + + No more errors occur when new storage volume is being created + using ``vol-create`` with ``--validate`` option.
Changes to NEWS must be always in a separate patch. Mainly you definitely do not want to have conflicts when backporting patches, where NEWS definitely do not match.
v11.2.0 (2025-04-01) ==================== diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 68842004b7..f6d804bb39 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1409,6 +1409,8 @@ virStorageVolDefParse(virStoragePoolDef *pool, "volume", &ctxt, "storagevol.rng", validate))) return NULL;
+ flags &= ~(VIR_STORAGE_VOL_CREATE_VALIDATE);
This function gets VIR_VOL_XML_PARSE_VALIDATE. In storageVolCreateXML VIR_STORAGE_VOL_CREATE_VALIDATE is converted to VIR_VOL_XML_PARSE_VALIDATE.
Where did VIR_STORAGE_VOL_CREATE_VALIDATE leak from?
Either way this hunk is incorrect as this flag should not get here and the caller needs to be fixed.
If in fact you meant to mask out VIR_VOL_XML_PARSE_VALIDATE here
Yeah, that was the initial plan.
I suggest you rather accept it in the virCheckFlags inside the parser code rather than masking it out.
Well, I was the one who suggested to Kirill to mask the flag out. Accepting the flag inside a function and then never using it just felt wrong. Michal
On Mon, Apr 07, 2025 at 15:45:24 +0200, Michal Prívozník wrote:
On 4/7/25 15:11, Peter Krempa via Devel wrote:
On Mon, Apr 07, 2025 at 14:49:14 +0200, Peter Krempa via Devel wrote:
On Mon, Apr 07, 2025 at 14:25:43 +0200, Kirill Shchetiniuk via Devel wrote:
[...]
Either way this hunk is incorrect as this flag should not get here and the caller needs to be fixed.
If in fact you meant to mask out VIR_VOL_XML_PARSE_VALIDATE here
Yeah, that was the initial plan.
I suggest you rather accept it in the virCheckFlags inside the parser code rather than masking it out.
Well, I was the one who suggested to Kirill to mask the flag out. Accepting the flag inside a function and then never using it just felt wrong.
Well there certainly are cases where we do accept a somewhat "useless" flag. Examples are e.g. VIR_TYPED_PARAM_STRING_OKAY in some of the APIs and VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC which is now pointless as we deleted the non-atomic snapshot code. My stance is that 'virCheckFlags' means "Following code handles $FLAGS properly."
On Mon, Apr 07, 2025 at 03:59:25PM +0200, Peter Krempa wrote:
On Mon, Apr 07, 2025 at 15:45:24 +0200, Michal Prívozník wrote:
On 4/7/25 15:11, Peter Krempa via Devel wrote:
On Mon, Apr 07, 2025 at 14:49:14 +0200, Peter Krempa via Devel wrote:
On Mon, Apr 07, 2025 at 14:25:43 +0200, Kirill Shchetiniuk via Devel wrote:
[...]
Either way this hunk is incorrect as this flag should not get here and the caller needs to be fixed.
If in fact you meant to mask out VIR_VOL_XML_PARSE_VALIDATE here
Yeah, that was the initial plan.
I suggest you rather accept it in the virCheckFlags inside the parser code rather than masking it out.
Well, I was the one who suggested to Kirill to mask the flag out. Accepting the flag inside a function and then never using it just felt wrong.
Well there certainly are cases where we do accept a somewhat "useless" flag. Examples are e.g. VIR_TYPED_PARAM_STRING_OKAY in some of the APIs and VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC which is now pointless as we deleted the non-atomic snapshot code.
My stance is that 'virCheckFlags' means "Following code handles $FLAGS properly."
As for me it is a bit pointless to have unused flags in 'virCheckFlags', at least in context of suggested changes. According to this logic we can just put every possible (not explicitly prohibited) flag inside any check, as following code does nothing with it and handles it properly, and as a result, in this particular case, flag check would make no sense at all.. In case of VIR_TYPED_PARAM_STRING_OKAY, it is fine as functions where this approach is used are a part of some hypervisor API, not only because it is required due to API unification, but also because caller do not perform any actions according to this flag. In our case caller perform required action and we do not need this flag to perform further actions. Any extra thoughts?
On Tue, Apr 08, 2025 at 20:21:13 +0200, Kirill Shchetiniuk wrote:
On Mon, Apr 07, 2025 at 03:59:25PM +0200, Peter Krempa wrote:
On Mon, Apr 07, 2025 at 15:45:24 +0200, Michal Prívozník wrote:
On 4/7/25 15:11, Peter Krempa via Devel wrote:
On Mon, Apr 07, 2025 at 14:49:14 +0200, Peter Krempa via Devel wrote:
On Mon, Apr 07, 2025 at 14:25:43 +0200, Kirill Shchetiniuk via Devel wrote:
[...]
Either way this hunk is incorrect as this flag should not get here and the caller needs to be fixed.
If in fact you meant to mask out VIR_VOL_XML_PARSE_VALIDATE here
Yeah, that was the initial plan.
I suggest you rather accept it in the virCheckFlags inside the parser code rather than masking it out.
Well, I was the one who suggested to Kirill to mask the flag out. Accepting the flag inside a function and then never using it just felt wrong.
Well there certainly are cases where we do accept a somewhat "useless" flag. Examples are e.g. VIR_TYPED_PARAM_STRING_OKAY in some of the APIs and VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC which is now pointless as we deleted the non-atomic snapshot code.
My stance is that 'virCheckFlags' means "Following code handles $FLAGS properly."
As for me it is a bit pointless to have unused flags in 'virCheckFlags', at least in context of suggested changes. According to this logic we can just put every possible (not explicitly prohibited) flag inside any check, as following code does nothing with it and handles it properly, and as a result, in this particular case, flag check would make no sense at all..
That is what's usually happening. You put in everything from the given flag enum that works properly with that function. At least in the vast majority of virCheckFlag use which is hypervisor API entry points
In case of VIR_TYPED_PARAM_STRING_OKAY, it is fine as functions where this approach is used are a part of some hypervisor API, not only because it is required due to API unification, but also because caller do not perform any actions according to this flag. In our case caller perform required action and we do not need this flag to perform further actions.
Ah, I see you are mentioning those. Well beware that the use of virCheckFlags outside of hypervisor APIs is really minimal.
Any extra thoughts?
Just make sure you use the proper flag with the check; I don't mind masking it out that much.
participants (3)
-
Kirill Shchetiniuk -
Michal Prívozník -
Peter Krempa