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?
