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