Re: [libvirt] [PATCH 1/4] qemu: Introduce qemuDomainAgentJob
[...]
>>
>> + /* The following members are for QEMU_AGENT_JOB_* */
>> + qemuDomainAgentJob agentActive; /* Currently running agent job */
>
> Hmm... I seem to recall a recent patch that spoke against using an enum
> type for struct fields:
>
> https://www.redhat.com/archives/libvir-list/2018-June/msg00486.html
>
> While I know you're just copying the existing qemuDomainJob - this is
> the kind of stuff that just gets copied around and used elsewhere.
No this is something different. The problem with using enums in
virDomainDef (or any other struct that is immediate part of XML parsing)
is that some of us use the following construct:
typedef enum {
VIR_SOME_ENUM_ITEM1,
VIR_SOME_ENUM_ITEM2,
...
VIR_SOME_ENUM_ITEM_LAST
} virSomeEnum;
virSomeEnum val = virSomeEnumTypeFromString(str);
The problem lies in the last line because TypeFromString() can return -1
(if @str does not fall into virSomeEnum). Therefore the last line can be
rewritten as:
virSomeEnum val = -1;
I guess you can see the problem now. There are basically two ways to get
rid of this problem. The first one is to use an intermediary integer:
virSomEnum val;
int tmp = virSomeEnumTypeFromString(str);
if (tmp < 0)
error;
else
val = tmp;
The other way is to move the integer into the struct and put into the
comment reference to the enum (this is what Dan is doing in the patch
you're referring to).
However, my scenario is different. Whenever setting the struct member
it's only using corresponding enum values.
I understand the difference, but was merely pointing out the dichotomy
in using an enum in a struct if one takes the comment of the linked
patch literally as much as they take the review comment literally.
I think usage of an enum in a struct is not a blanket don't do that, but
rather a well we can do it for certain situations that don't involve use
of switches and/or *TypeFrom* processing.
>
>> + unsigned long long agentOwner; /* Thread id which set current agent job
*/
>> + const char *agentOwnerAPI; /* The API which owns the agent job */
>> + unsigned long long agentStarted; /* When the current agent job started
*/
>> +
>
> FWIW: From the description in the next patch, I would think there would
> need to be an agentCond variable too. But reading patch 4 I know that's
> not quite the case. I have concerns over the combined @cond usage, but
> those are only towards some "future change" that misses some subtlety.
>
No. Using one condition is the point of these patches. IMO having two
conditions would only complicate things needlessly. The code is guarded
against "spurious" wakeups from condition.
Yes, I came to understand that, but it's the THREADS.txt description
that probably needs adjustment. Remember you're working backwards and
working forwards - e.g. you already know and I'm learning from review.
If you take that viewpoint when reading THREADS.txt what do you get
(e.g. the more literal viewpoint). The whole relationship with async
jobs (and your other series) just adds a level of complexity.
John