7:25 a.m.
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(a)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
7:49 a.m.
New subject: [PATCH] storage: virStorageVolDefParse and storageVolCreateXML
flags fix
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(a)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.
8:11 a.m.
New subject: [PATCH] storage: virStorageVolDefParse and storageVolCreateXML
flags fix
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(a)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.
8:25 a.m.
New subject: [PATCH] storage: virStorageVolDefParse and storageVolCreateXML
flags fix
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(a)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(a)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.
8:55 a.m.
New subject: [PATCH] storage: virStorageVolDefParse and storageVolCreateXML
flags fix
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(a)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(a)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?
8:45 a.m.
New subject: [PATCH] storage: virStorageVolDefParse and storageVolCreateXML
flags fix
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(a)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
8:59 a.m.
New subject: [PATCH] storage: virStorageVolDefParse and storageVolCreateXML
flags fix
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."
1:21 p.m.
New subject: [PATCH] storage: virStorageVolDefParse and storageVolCreateXML
flags fix
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?
2:25 p.m.
New subject: [PATCH] storage: virStorageVolDefParse and storageVolCreateXML
flags fix
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.
20
days inactive
21
days old
8 comments
3 participants
participants (3)
-
Kirill Shchetiniuk -
Michal Prívozník -
Peter Krempa