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@xxxxxxxxxx>
>>> ---
>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
