Re: [Outreachy kernel] [PATCH] staging: unisys: Modify boolean assignment

Re: [Outreachy kernel] [PATCH] staging: unisys: Modify boolean assignment

[Julia Lawall]

On Wed, 10 Feb 2016, Janani Ravichandran wrote:

> Boolean variables should be assigned true/false rather than 1/0.
> This patch makes such a correction.

A change like this should be made consistently, so it is a little 
surprising to see only one such change.  Is this the only place where the 
variable is initialized?  If so, you could say that.  if not, then you 
could explain that all of the other initialization places already have 
boolean values, in some way or another.

julia

> 
> Signed-off-by: Janani Ravichandran <janani.rvchndrn@gmail.com>
> ---
>  drivers/staging/unisys/visorhba/visorhba_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c b/drivers/staging/unisys/visorhba/visorhba_main.c
> index 5a78409..e93bb1d 100644
> --- a/drivers/staging/unisys/visorhba/visorhba_main.c
> +++ b/drivers/staging/unisys/visorhba/visorhba_main.c
> @@ -1062,7 +1062,7 @@ static int visorhba_resume(struct visor_device *dev,
>  		return -EINVAL;
>  
>  	if (devdata->serverdown && !devdata->serverchangingstate)
> -		devdata->serverchangingstate = 1;
> +		devdata->serverchangingstate = true;
>  
>  	visor_thread_start(&devdata->threadinfo, process_incoming_rsps,
>  			   devdata, "vhba_incming");
> -- 
> 2.5.0
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160210082412.GA2645%40janani-Inspiron-3521.
> For more options, visit https://groups.google.com/d/optout.
> 

Re: [Outreachy kernel] [PATCH] staging: unisys: Modify boolean assignment

[Janani Ravichandran]

[-- Attachment #1.1: Type: text/plain, Size: 2346 bytes --]



On Wednesday, 10 February 2016 02:26:38 UTC-5, Julia Lawall wrote:
>
> On Wed, 10 Feb 2016, Janani Ravichandran wrote: 
>
> > Boolean variables should be assigned true/false rather than 1/0. 
> > This patch makes such a correction. 
>
> A change like this should be made consistently, so it is a little 
> surprising to see only one such change.  Is this the only place where the 
> variable is initialized?  

Within this file, this is the only instance of initialization of the 
variable where 1 is
 used instead of true. All others use true/false. I sent this to resolve 
that 
inconsistency :) 

> If so, you could say that.  if not, then you 
> could explain that all of the other initialization places already have 
> boolean values, in some way or another. 
>
Do I send a v2 explaining this? 

>
> julia 
>
> > 
> > Signed-off-by: Janani Ravichandran <janani....@gmail.com <javascript:>> 
> > --- 
> >  drivers/staging/unisys/visorhba/visorhba_main.c | 2 +- 
> >  1 file changed, 1 insertion(+), 1 deletion(-) 
> > 
> > diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c 
> b/drivers/staging/unisys/visorhba/visorhba_main.c 
> > index 5a78409..e93bb1d 100644 
> > --- a/drivers/staging/unisys/visorhba/visorhba_main.c 
> > +++ b/drivers/staging/unisys/visorhba/visorhba_main.c 
> > @@ -1062,7 +1062,7 @@ static int visorhba_resume(struct visor_device 
> *dev, 
> >                  return -EINVAL; 
> >   
> >          if (devdata->serverdown && !devdata->serverchangingstate) 
> > -                devdata->serverchangingstate = 1; 
> > +                devdata->serverchangingstate = true; 
> >   
> >          visor_thread_start(&devdata->threadinfo, process_incoming_rsps, 
> >                             devdata, "vhba_incming"); 
> > -- 
> > 2.5.0 
> > 
> > -- 
> > You received this message because you are subscribed to the Google 
> Groups "outreachy-kernel" group. 
> > To unsubscribe from this group and stop receiving emails from it, send 
> an email to outreachy-kern...@googlegroups.com <javascript:>. 
> > To post to this group, send email to outreach...@googlegroups.com 
> <javascript:>. 
> > To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/20160210082412.GA2645%40janani-Inspiron-3521. 
>
> > For more options, visit https://groups.google.com/d/optout. 
> > 
>

[-- Attachment #1.2: Type: text/html, Size: 4424 bytes --]

[PATCH] staging: unisys: Modify boolean assignment

[Janani Ravichandran]

Boolean variables should be assigned true/false rather than 1/0.
This patch makes such a correction.

Signed-off-by: Janani Ravichandran <janani.rvchndrn@gmail.com>
---
 drivers/staging/unisys/visorhba/visorhba_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c b/drivers/staging/unisys/visorhba/visorhba_main.c
index 5a78409..e93bb1d 100644
--- a/drivers/staging/unisys/visorhba/visorhba_main.c
+++ b/drivers/staging/unisys/visorhba/visorhba_main.c
@@ -1062,7 +1062,7 @@ static int visorhba_resume(struct visor_device *dev,
 		return -EINVAL;
 
 	if (devdata->serverdown && !devdata->serverchangingstate)
-		devdata->serverchangingstate = 1;
+		devdata->serverchangingstate = true;
 
 	visor_thread_start(&devdata->threadinfo, process_incoming_rsps,
 			   devdata, "vhba_incming");
-- 
2.5.0

Re: [Outreachy kernel] [PATCH] staging: unisys: Modify boolean assignment

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3308 bytes --]



On Tue, 9 Feb 2016, Janani Ravichandran wrote:

>
>
> On Wednesday, 10 February 2016 02:26:38 UTC-5, Julia Lawall wrote:
>       On Wed, 10 Feb 2016, Janani Ravichandran wrote:
>
>       > Boolean variables should be assigned true/false rather than
>       1/0.
>       > This patch makes such a correction.
>
>       A change like this should be made consistently, so it is a
>       little
>       surprising to see only one such change.  Is this the only place
>       where the
>       variable is initialized?  
>
> Within this file, this is the only instance of initialization of the
> variable where 1 is
>  used instead of true. All others use true/false. I sent this to resolve
> that
> inconsistency :)
>       If so, you could say that.  if not, then you
>       could explain that all of the other initialization places
>       already have
>       boolean values, in some way or another.
>
> Do I send a v2 explaining this?

I think it could be helpful.

julia

>
>       julia
>
>       >
>       > Signed-off-by: Janani Ravichandran <janani....@gmail.com>
>       > ---
>       >  drivers/staging/unisys/visorhba/visorhba_main.c | 2 +-
>       >  1 file changed, 1 insertion(+), 1 deletion(-)
>       >
>       > diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c
>       b/drivers/staging/unisys/visorhba/visorhba_main.c
>       > index 5a78409..e93bb1d 100644
>       > --- a/drivers/staging/unisys/visorhba/visorhba_main.c
>       > +++ b/drivers/staging/unisys/visorhba/visorhba_main.c
>       > @@ -1062,7 +1062,7 @@ static int visorhba_resume(struct
>       visor_device *dev,
>       >                  return -EINVAL;
>       >  
>       >          if (devdata->serverdown &&
>       !devdata->serverchangingstate)
>       > -                devdata->serverchangingstate = 1;
>       > +                devdata->serverchangingstate = true;
>       >  
>       >          visor_thread_start(&devdata->threadinfo,
>       process_incoming_rsps,
>       >                             devdata, "vhba_incming");
>       > --
>       > 2.5.0
>       >
>       > --
>       > You received this message because you are subscribed to the
>       Google Groups "outreachy-kernel" group.
>       > To unsubscribe from this group and stop receiving emails from
>       it, send an email to outreachy-kern...@googlegroups.com.
>       > To post to this group, send email to
>       outreach...@googlegroups.com.
>       > To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/20160210082412.GA2645%40
>       janani-Inspiron-3521.
>       > For more options, visit https://groups.google.com/d/optout.
>       >
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/207e6b2c-07b0-48a8-a414-
> 37c59ed7a3e6%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
>

Re: [Outreachy kernel] [PATCH] staging: unisys: Modify boolean assignment

[Jes Sorensen]

On 02/10/16 03:54, Julia Lawall wrote:
> 
> 
> On Tue, 9 Feb 2016, Janani Ravichandran wrote:
> 
>>
>>
>> On Wednesday, 10 February 2016 02:26:38 UTC-5, Julia Lawall wrote:
>>       On Wed, 10 Feb 2016, Janani Ravichandran wrote:
>>
>>       > Boolean variables should be assigned true/false rather than
>>       1/0.
>>       > This patch makes such a correction.
>>
>>       A change like this should be made consistently, so it is a
>>       little
>>       surprising to see only one such change.  Is this the only place
>>       where the
>>       variable is initialized?  
>>
>> Within this file, this is the only instance of initialization of the
>> variable where 1 is
>>  used instead of true. All others use true/false. I sent this to resolve
>> that
>> inconsistency :)
>>       If so, you could say that.  if not, then you
>>       could explain that all of the other initialization places
>>       already have
>>       boolean values, in some way or another.
>>
>> Do I send a v2 explaining this?
> 
> I think it could be helpful.

You also need to CC the driver author, not just the outreachy list.

Jes

Re: [Outreachy kernel] [PATCH] staging: unisys: Modify boolean assignment

[Jes Sorensen]

On 02/17/16 13:03, Greg KH wrote:
> On Wed, Feb 17, 2016 at 12:59:49PM -0500, Jes Sorensen wrote:
>> On 02/10/16 03:54, Julia Lawall wrote:
>>>
>>>
>>> On Tue, 9 Feb 2016, Janani Ravichandran wrote:
>>>
>>>>
>>>>
>>>> On Wednesday, 10 February 2016 02:26:38 UTC-5, Julia Lawall wrote:
>>>>       On Wed, 10 Feb 2016, Janani Ravichandran wrote:
>>>>
>>>>       > Boolean variables should be assigned true/false rather than
>>>>       1/0.
>>>>       > This patch makes such a correction.
>>>>
>>>>       A change like this should be made consistently, so it is a
>>>>       little
>>>>       surprising to see only one such change.  Is this the only place
>>>>       where the
>>>>       variable is initialized?  
>>>>
>>>> Within this file, this is the only instance of initialization of the
>>>> variable where 1 is
>>>>  used instead of true. All others use true/false. I sent this to resolve
>>>> that
>>>> inconsistency :)
>>>>       If so, you could say that.  if not, then you
>>>>       could explain that all of the other initialization places
>>>>       already have
>>>>       boolean values, in some way or another.
>>>>
>>>> Do I send a v2 explaining this?
>>>
>>> I think it could be helpful.
>>
>> You also need to CC the driver author, not just the outreachy list.
> 
> For the outreachy application process, the author doesn't need to be
>  cc:ed, but they can be if the patch author wants to.

If the patches are dropped and not pushed upstream, please ignore CC's,
sure. If they are meant to go upstream, the author must be CC'ed.

I for one find it really annoying and rude to have patches go into
upstream for code I actively maintain, without being CC'ed on them.

I know Unisys is trying to fix up their code, and knowing conflicting
patches are in flight saves work.

Jes

Re: [Outreachy kernel] [PATCH] staging: unisys: Modify boolean assignment

[Greg KH]

On Wed, Feb 17, 2016 at 01:10:15PM -0500, Jes Sorensen wrote:
> On 02/17/16 13:03, Greg KH wrote:
> > On Wed, Feb 17, 2016 at 12:59:49PM -0500, Jes Sorensen wrote:
> >> On 02/10/16 03:54, Julia Lawall wrote:
> >>>
> >>>
> >>> On Tue, 9 Feb 2016, Janani Ravichandran wrote:
> >>>
> >>>>
> >>>>
> >>>> On Wednesday, 10 February 2016 02:26:38 UTC-5, Julia Lawall wrote:
> >>>>       On Wed, 10 Feb 2016, Janani Ravichandran wrote:
> >>>>
> >>>>       > Boolean variables should be assigned true/false rather than
> >>>>       1/0.
> >>>>       > This patch makes such a correction.
> >>>>
> >>>>       A change like this should be made consistently, so it is a
> >>>>       little
> >>>>       surprising to see only one such change.  Is this the only place
> >>>>       where the
> >>>>       variable is initialized?  
> >>>>
> >>>> Within this file, this is the only instance of initialization of the
> >>>> variable where 1 is
> >>>>  used instead of true. All others use true/false. I sent this to resolve
> >>>> that
> >>>> inconsistency :)
> >>>>       If so, you could say that.  if not, then you
> >>>>       could explain that all of the other initialization places
> >>>>       already have
> >>>>       boolean values, in some way or another.
> >>>>
> >>>> Do I send a v2 explaining this?
> >>>
> >>> I think it could be helpful.
> >>
> >> You also need to CC the driver author, not just the outreachy list.
> > 
> > For the outreachy application process, the author doesn't need to be
> >  cc:ed, but they can be if the patch author wants to.
> 
> If the patches are dropped and not pushed upstream, please ignore CC's,
> sure. If they are meant to go upstream, the author must be CC'ed.
> 
> I for one find it really annoying and rude to have patches go into
> upstream for code I actively maintain, without being CC'ed on them.

That happens all the time, and is just the way things have always been
for kernel changes.  Maintainership isn't "absolute" ownership, you know
this :)

> I know Unisys is trying to fix up their code, and knowing conflicting
> patches are in flight saves work.

These should all be trivial to work around, and really, if they were
trying to fix up their code, they would have already resolved "trivial"
changes like this a long time ago.

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH] staging: unisys: Modify boolean assignment

[Jes Sorensen]

On 02/20/16 18:30, Greg KH wrote:
> On Wed, Feb 17, 2016 at 01:10:15PM -0500, Jes Sorensen wrote:
>> On 02/17/16 13:03, Greg KH wrote:
>>> On Wed, Feb 17, 2016 at 12:59:49PM -0500, Jes Sorensen wrote:
>>>> You also need to CC the driver author, not just the outreachy list.
>>>
>>> For the outreachy application process, the author doesn't need to be
>>>  cc:ed, but they can be if the patch author wants to.
>>
>> If the patches are dropped and not pushed upstream, please ignore CC's,
>> sure. If they are meant to go upstream, the author must be CC'ed.
>>
>> I for one find it really annoying and rude to have patches go into
>> upstream for code I actively maintain, without being CC'ed on them.
> 
> That happens all the time, and is just the way things have always been
> for kernel changes.  Maintainership isn't "absolute" ownership, you know
> this :)

Of course, however it's common courtesy to CC a maintainer. If the code
has been untouched for a long time, sure, but if it's actively being
developed that is a different story.

>> I know Unisys is trying to fix up their code, and knowing conflicting
>> patches are in flight saves work.
> 
> These should all be trivial to work around, and really, if they were
> trying to fix up their code, they would have already resolved "trivial"
> changes like this a long time ago.

It really depends, checkpatch claims errors on a lot of things that
aren't broken in the first place. It is also a question of what to fix
first, real bugs or moving commas around. Either way, this is no excuse
for not being courteous and CC'ing the maintainer on the changes.

Jes