Re: [PATCH] i2cset: Fix short writes with mask

[Jean Delvare <jdelvare@suse.de>]

On Tue, 8 Sep 2020 08:51:11 +0200, Wolfram Sang wrote:
> On Thu, Sep 03, 2020 at 11:00:54AM +0200, Jean Delvare wrote:
> > Short writes used "daddress" for the value, but the masking code did
> > not expect that, and instead applied the mask to a variable that was
> > never used.
> > 
> > So change short writes to use "value" for the value, as all other
> > commands do. Adjust all code paths accordingly.
> > 
> > Reported by David Jedynak.
> > 
> > Signed-off-by: Jean Delvare <jdelvare@suse.de>  
> 
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Passed my test as well. One question:

Thank you very much for testing! I'll commit that now.

> > @@ -140,7 +140,7 @@ static int confirm(const char *filename,
> >  	} else
> >  		fprintf(stderr, "data 0x%02x%s, mode %s.\n", value,
> >  			vmask ? " (masked)" : "",
> > -			size == I2C_SMBUS_BYTE_DATA ? "byte" : "word");
> > +			size == I2C_SMBUS_WORD_DATA ? "word" : "byte");  
> 
> Does this really change something or is it a refactoring leftover?

It does change something. Before the changes, size could only have two
values here, I2C_SMBUS_BYTE_DATA and I2C_SMBUS_WORD_DATA, so the result
would indeed be the same. But after the changes, size can also have
value I2C_SMBUS_BYTE, for which we want to display "mode byte", not
"mode word". Which is why we test explicitly for I2C_SMBUS_WORD_DATA,
else default to "byte", rather than the other way around.

Note that this also aligns nicely with the rest of the code in this
file, which does the same in various places.

Funny story, while I only posted this last week, I wrote the fix
several months ago, so last week I actually got to review my own code
with fresh eyes, and when I stumbled upon that specific change, first
thing that came to my mind was "this is a useless change, why did I do
that ?" Then I scrolled up, checked the other changes in that function,
noticed the removed "else" and said OK, I'm not that stupid after all
;-)

-- 
Jean Delvare
SUSE L3 Support