Re: [libvirt] [PATCH 2/4] conf: domain: Introduce virDomainDeviceInfoIterate flags
On 12/12/18 3:29 AM, Erik Skultety wrote:
On Tue, Dec 11, 2018 at 06:50:19PM -0500, John Ferlan wrote:
>
>
> On 12/7/18 9:47 AM, Erik Skultety wrote:
>> One of the usages of the device iterator is to run config validation.
>> That's a problem for graphics devices, because they don't have any @info
>> data (graphics shouldn't have been considered as devices in the first
>> place), and simply passing NULL would crash a few callbacks invoked from
>> the iterator. Fix this problem by introducing iterator flags.
>
> Somewhat confusing commit message or I'm just being dense, your choice.
I see, especially the mention of graphics devices in a patch where there's
nothing related, how about this:
Validation of domain devices is accomplished via a generic device iterator
which takes a callback, iterates over all kinds of supported device types and
invokes the callback on every single device. However, there might be cases when
we need to alter the behaviour of the iteration (most notably skip or include a
group of devices). Therefore, this patch introduces iterator flags.
That's fine.
>
> Although I think that @info data for graphics devices could be moved
> into patch4 since that's where it makes more sense (eventually at least).
>
>>
>> Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
>> ---
>> src/conf/domain_conf.c | 27 ++++++++++++++++++++-------
>> 1 file changed, 20 insertions(+), 7 deletions(-)
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index b70dca6c61..11552bff5b 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -3703,10 +3703,18 @@ virDomainSkipBackcompatConsole(virDomainDefPtr def,
>> }
>>
>>
>> +typedef enum {
>> + DEVICE_INFO_ITERATE_ALL_CONSOLES = 1 << 0, /* Iterate console[0] */
>> +} virDomainDeviceInfoIterateFlags;
>> +
>
> Logic seems right vis-a-vis replacing a bool with a flag properly. I
> don't get the name and comment, but I'm not sure I could name it better.
The iterator function itself doesn't have a perfect name, I was just trying to
use that name to derive a new one to reflect the relation, but I can use
virDomainDeviceIteratorFlags instead. Going one step further we might even want
to rename the iterator and drop the "Info" part since that won't be 100%
once I
enable it for graphics.
<facepalm> I meant just the flag name itself - the typedef name is just
that and used only once, so no big deal.
> The called function with the @all flag only processes when @all
== false
So, because console[0] corresponds to the first serial console, we often
special-case these (in general). The @all flag here just says whether the
processing callback should skip console[0] or not, if @all == false, then
virDomainSkipBackcompatConsole will return true, thus skipping invocation of
the callback on this device.
Yeah I recall the [0] console entry being quite special with the details
being found in various places in the code.
In any case, if @all == false, then true is only returned if other
conditions are met too such as using/checking the [0]th entry of the
consoles list. When @all == true, then false is returned and that's the
"more normal" case as prior to this patch @all was true for all except
when virDomainDeviceInfoIterate calls virDomainDeviceInfoIterateInternal
> as I'm reading things. Anyway, if someone has a better idea, then they
> should speak up before you push!
Let me know whether the suggested update to the commit message along with the
changes to naming are okay and I'll proceed with merging the patch.
Sure things are fine
John