[libvirt] [PATCH] conf: check rombar against VIR_DOMAIN_TRISTATE_SWITCH_ABSENT

[Pavel Hrdina]

On Mon, Aug 07, 2017 at 05:50:51PM +0200, Michal Privoznik wrote:
> On 08/07/2017 05:30 PM, Pavel Hrdina wrote:
> > On Mon, Aug 07, 2017 at 05:06:49PM +0200, Michal Privoznik wrote:
> >> On 08/07/2017 04:56 PM, Ján Tomko wrote:
> >>> Make the comparison explicit.
> >>> ---
> >>>  src/conf/domain_conf.c | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> >>> index 3cdb5e348..b5ce2ecd9 100644
> >>> --- a/src/conf/domain_conf.c
> >>> +++ b/src/conf/domain_conf.c
> >>> @@ -5359,10 +5359,10 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
> >>>      }
> >>>  
> >>>      if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM) &&
> >>> -        (info->rombar || info->romfile)) {
> >>> +        (info->rombar != VIR_TRISTATE_SWITCH_ABSENT || info->romfile)) {
> >>>  
> >>>          virBufferAddLit(buf, "<rom");
> >>> -        if (info->rombar) {
> >>> +        if (info->rombar != VIR_TRISTATE_SWITCH_ABSENT) {
> >>>              const char *rombar = virTristateSwitchTypeToString(info->rombar);
> >>>  
> >>>              if (rombar)
> >>>
> >>
> >> I'm not against this patch, it's just that we set ABSENT explicitly to
> >> zero value so that we can do shortcuts like this. If we don't want to
> >> have them, we ought to remove the explicit value assignment.
> > 
> > The shortcut is nice, but I don't like it personally.  If the variable
> > can contain more than two states I'd rather check it explicitly. 
> 
> So what's the point of assigning _ABSENT zero value then?
> 
> > That's
> > why I prefer (int == 0) over (!int).
> 
> Well, if this is a part of bigger statement then yes, for instance:
> 
> if (x == 0) {
> } else if (x == 1) {
> } else {
> }
> 
> (although, sometimes we might prefer switch() for that). But if it's
> just a simple check whether a value was set or is equal to some default
> (= if the check is interested in distinguishing just two states anyway),
> !var works for me too:
> 
> if (x)
>    formatToXML(x)

IMHO we should use the same form for all checks.

> 
> But sure, var == 0 vs !var is a personal preference. The important part
> is my first question. If we dislike these shortcuts (in either of their
> form), shouldn't we just drop explicit value assignment in the enum?

The point is that the value 0 is still named as _ABSENT.  The benefit
of not using the shortcut for the VIR_TRISTATE_*_ABSENT is that you know
right away what the variable stores and also you now right away that
this part of code is executed only if the tristate was set to something.

Yes, it this particular case you can see few lines below that there is
virTristateSwitchTypeToString() which gives you hint what the variable
is, but it's not always that way.

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170808/c138b753/attachment-0001.sig>