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.
