Re: [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?