1. Patchew
  2. Libvirt
  3. View series

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

Ján Tomko posted 1 patch 6 years, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/8648a12278142d6338701501b13509affbd2eb33.1502117761.git.jtomko@redhat.com
src/conf/domain_conf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[libvirt] [PATCH] conf: check rombar against VIR_DOMAIN_TRISTATE_SWITCH_ABSENT
Posted by Ján Tomko 6 years, 8 months ago 
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)
-- 
2.13.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: check rombar against VIR_DOMAIN_TRISTATE_SWITCH_ABSENT
Posted by Pavel Hrdina 6 years, 8 months ago 
On Mon, Aug 07, 2017 at 04:56:07PM +0200, Ján Tomko wrote:
> Make the comparison explicit.
> ---
>  src/conf/domain_conf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: check rombar against VIR_DOMAIN_TRISTATE_SWITCH_ABSENT
Posted by Michal Privoznik 6 years, 8 months ago 
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.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: check rombar against VIR_DOMAIN_TRISTATE_SWITCH_ABSENT
Posted by Pavel Hrdina 6 years, 8 months ago 
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.  That's
why I prefer (int == 0) over (!int).

Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: check rombar against VIR_DOMAIN_TRISTATE_SWITCH_ABSENT
Posted by Michal Privoznik 6 years, 8 months ago 
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)

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?

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: check rombar against VIR_DOMAIN_TRISTATE_SWITCH_ABSENT
Posted by Pavel Hrdina 6 years, 8 months ago 
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: check rombar against VIR_DOMAIN_TRISTATE_SWITCH_ABSENT
Posted by Michal Privoznik 6 years, 8 months ago 
On 08/08/2017 10:49 AM, Pavel Hrdina wrote:
> 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.

This is argument for being explicit about checks (which I'm not
objecting to, I said at the beginning that I'm up for it). The thing I'm
interested in is: if we are explicit in all the places, what's the point
in having explicit assignment _ABSENT = 0? We have some enums without
explicit value assignment to any member. The only reason I can think of
is that when we alloc new struct, the memory chunk is prefilled with
zeroes so we don't have to do explicit setting to _ABSENT.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.