| Message ID | 20141212192703.GA22234@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Dec 13 2014 04:27, Dan Carpenter wrote: > This code tends to use unsigned variables by default and it causes > signedness bugs when we use negative variables for error handling. > The "i" and "j" variables are used to iterated over small positive > values and so they should be type "int". The "len" variable doesn't > *need* to be signed but it should be signed to make the code easier to > read and audit. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Thanks, but I prefer to use 'unsigned int' for loop counter, like the other drivers. Would you give 'int' type just for err variables? Regards Takashi Sakamoto o-takashi@sakamocchi.jp
On Sat, Dec 13, 2014 at 02:06:59PM +0900, Takashi Sakamoto wrote: > On Dec 13 2014 04:27, Dan Carpenter wrote: > > This code tends to use unsigned variables by default and it causes > > signedness bugs when we use negative variables for error handling. > > The "i" and "j" variables are used to iterated over small positive > > values and so they should be type "int". The "len" variable doesn't > > *need* to be signed but it should be signed to make the code easier to > > read and audit. > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > Thanks, but I prefer to use 'unsigned int' for loop counter, like the > other drivers. Unthinking use of unsigned int every where is a kind of leprosy. It just makes the code hard to read and it causes bugs. regards, dan carpenter
On Dec 13 2014 16:04, Dan Carpenter wrote: > On Sat, Dec 13, 2014 at 02:06:59PM +0900, Takashi Sakamoto wrote: >> On Dec 13 2014 04:27, Dan Carpenter wrote: >>> This code tends to use unsigned variables by default and it causes >>> signedness bugs when we use negative variables for error handling. >>> The "i" and "j" variables are used to iterated over small positive >>> values and so they should be type "int". The "len" variable doesn't >>> *need* to be signed but it should be signed to make the code easier to >>> read and audit. >>> >>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >> >> Thanks, but I prefer to use 'unsigned int' for loop counter, like the >> other drivers. > > Unthinking use of unsigned int every where is a kind of leprosy. It > just makes the code hard to read and it causes bugs. In the beginning of last year, I received a comment from Clemens to use 'unsigned int' type for loop counter instead of 'int' type. Since then, for ALSA firewire stack, I've been following his advice. (but actually I missed to OXFW driver...) For consistency inner this stack, I request you to follow this. Regards Takashi Sakamoto o-takashi@sakamocchi.jp
On Sat, Dec 13, 2014 at 07:14:46PM +0900, Takashi Sakamoto wrote: > On Dec 13 2014 16:04, Dan Carpenter wrote: > > On Sat, Dec 13, 2014 at 02:06:59PM +0900, Takashi Sakamoto wrote: > >> On Dec 13 2014 04:27, Dan Carpenter wrote: > >>> This code tends to use unsigned variables by default and it causes > >>> signedness bugs when we use negative variables for error handling. > >>> The "i" and "j" variables are used to iterated over small positive > >>> values and so they should be type "int". The "len" variable doesn't > >>> *need* to be signed but it should be signed to make the code easier to > >>> read and audit. > >>> > >>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > >> > >> Thanks, but I prefer to use 'unsigned int' for loop counter, like the > >> other drivers. > > > > Unthinking use of unsigned int every where is a kind of leprosy. It > > just makes the code hard to read and it causes bugs. > > In the beginning of last year, I received a comment from Clemens to use > 'unsigned int' type for loop counter instead of 'int' type. Since then, > for ALSA firewire stack, I've been following his advice. (but actually I > missed to OXFW driver...) > > For consistency inner this stack, I request you to follow this. > That's nonsense advice. How many bugs has it prevented? It causes a lot. Whatever, just fix it and give me the reported-by tag. regards, dan carpenter
Think about a for loop:
for (i = 0; i < xxx; i++) {
If "i" is really so high that it goes negative you have a very serious
issue whether it's signed or unsigned. If it's signed then the loop is
a no-op but if it's unsigned then it corrupts memory.
That's not a win.
regards,
dan carpenter
At Sat, 13 Dec 2014 19:14:46 +0900, Takashi Sakamoto wrote: > > On Dec 13 2014 16:04, Dan Carpenter wrote: > > On Sat, Dec 13, 2014 at 02:06:59PM +0900, Takashi Sakamoto wrote: > >> On Dec 13 2014 04:27, Dan Carpenter wrote: > >>> This code tends to use unsigned variables by default and it causes > >>> signedness bugs when we use negative variables for error handling. > >>> The "i" and "j" variables are used to iterated over small positive > >>> values and so they should be type "int". The "len" variable doesn't > >>> *need* to be signed but it should be signed to make the code easier to > >>> read and audit. > >>> > >>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > >> > >> Thanks, but I prefer to use 'unsigned int' for loop counter, like the > >> other drivers. > > > > Unthinking use of unsigned int every where is a kind of leprosy. It > > just makes the code hard to read and it causes bugs. > > In the beginning of last year, I received a comment from Clemens to use > 'unsigned int' type for loop counter instead of 'int' type. Since then, > for ALSA firewire stack, I've been following his advice. (but actually I > missed to OXFW driver...) > > For consistency inner this stack, I request you to follow this. Note that Dan's suggestions are about the variables like "i" and "j". These are used normally for small loop counts, and they are int in a standard idiom. If they are declared as another type, you force reader's attention *unnecessarily*, and it decreases the readability (i.e. they have to read the loop code as somewhat special even if it's a normal loop). This is the only big drawback, and the rest merit/demerit are almost ignorable, IMO. Of course, in some cases, a loop count might be better in unsigned. But then a different variable name should be used instead. After all, this is rather a minor issue, almost a bikeshed topic, so I didn't care much while reviewing your patches, and I still don't care whether this fix patch will have int or unsigned for i. But, it'd be good if you keep this information in your mind, at least. thanks, Takashi
On Dec 14 2014 02:30, Takashi Iwai wrote: > At Sat, 13 Dec 2014 19:14:46 +0900, > Takashi Sakamoto wrote: >> >> On Dec 13 2014 16:04, Dan Carpenter wrote: >>> On Sat, Dec 13, 2014 at 02:06:59PM +0900, Takashi Sakamoto wrote: >>>> On Dec 13 2014 04:27, Dan Carpenter wrote: >>>>> This code tends to use unsigned variables by default and it causes >>>>> signedness bugs when we use negative variables for error handling. >>>>> The "i" and "j" variables are used to iterated over small positive >>>>> values and so they should be type "int". The "len" variable doesn't >>>>> *need* to be signed but it should be signed to make the code easier to >>>>> read and audit. >>>>> >>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > Note that Dan's suggestions are about the variables like "i" and "j". > These are used normally for small loop counts, and they are int in a > standard idiom. If they are declared as another type, you force > reader's attention *unnecessarily*, and it decreases the readability > (i.e. they have to read the loop code as somewhat special even if > it's a normal loop). This is the only big drawback, and the rest > merit/demerit are almost ignorable, IMO. > > Of course, in some cases, a loop count might be better in unsigned. > But then a different variable name should be used instead. > > After all, this is rather a minor issue, almost a bikeshed topic, so I > didn't care much while reviewing your patches, and I still don't care > whether this fix patch will have int or unsigned for i. But, it'd be > good if you keep this information in your mind, at least. Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> Thanks Takashi Sakamoto o-takashi@sakamocchi.jp
At Mon, 15 Dec 2014 02:01:18 +0900, Takashi Sakamoto wrote: > > On Dec 14 2014 02:30, Takashi Iwai wrote: > > At Sat, 13 Dec 2014 19:14:46 +0900, > > Takashi Sakamoto wrote: > >> > >> On Dec 13 2014 16:04, Dan Carpenter wrote: > >>> On Sat, Dec 13, 2014 at 02:06:59PM +0900, Takashi Sakamoto wrote: > >>>> On Dec 13 2014 04:27, Dan Carpenter wrote: > >>>>> This code tends to use unsigned variables by default and it causes > >>>>> signedness bugs when we use negative variables for error handling. > >>>>> The "i" and "j" variables are used to iterated over small positive > >>>>> values and so they should be type "int". The "len" variable doesn't > >>>>> *need* to be signed but it should be signed to make the code easier to > >>>>> read and audit. > >>>>> > >>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > Note that Dan's suggestions are about the variables like "i" and "j". > > These are used normally for small loop counts, and they are int in a > > standard idiom. If they are declared as another type, you force > > reader's attention *unnecessarily*, and it decreases the readability > > (i.e. they have to read the loop code as somewhat special even if > > it's a normal loop). This is the only big drawback, and the rest > > merit/demerit are almost ignorable, IMO. > > > > Of course, in some cases, a loop count might be better in unsigned. > > But then a different variable name should be used instead. > > > > After all, this is rather a minor issue, almost a bikeshed topic, so I > > didn't care much while reviewing your patches, and I still don't care > > whether this fix patch will have int or unsigned for i. But, it'd be > > good if you keep this information in your mind, at least. > > Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> OK, merged now. Thanks. Takashi
diff --git a/sound/firewire/oxfw/oxfw-proc.c b/sound/firewire/oxfw/oxfw-proc.c index 604808e..8ba4f9f2 100644 --- a/sound/firewire/oxfw/oxfw-proc.c +++ b/sound/firewire/oxfw/oxfw-proc.c @@ -15,7 +15,7 @@ static void proc_read_formation(struct snd_info_entry *entry, struct snd_oxfw_stream_formation formation, curr; u8 *format; char flag; - unsigned int i, err; + int i, err; /* Show input. */ err = snd_oxfw_stream_get_current_formation(oxfw, diff --git a/sound/firewire/oxfw/oxfw-stream.c b/sound/firewire/oxfw/oxfw-stream.c index b77cf80..bda845a 100644 --- a/sound/firewire/oxfw/oxfw-stream.c +++ b/sound/firewire/oxfw/oxfw-stream.c @@ -61,7 +61,8 @@ static int set_stream_format(struct snd_oxfw *oxfw, struct amdtp_stream *s, u8 **formats; struct snd_oxfw_stream_formation formation; enum avc_general_plug_dir dir; - unsigned int i, err, len; + unsigned int len; + int i, err; if (s == &oxfw->tx_stream) { formats = oxfw->tx_stream_formats; diff --git a/sound/firewire/oxfw/oxfw-pcm.c b/sound/firewire/oxfw/oxfw-pcm.c index 9bc556b..67ade07 100644 --- a/sound/firewire/oxfw/oxfw-pcm.c +++ b/sound/firewire/oxfw/oxfw-pcm.c @@ -19,7 +19,7 @@ static int hw_rule_rate(struct snd_pcm_hw_params *params, .min = UINT_MAX, .max = 0, .integer = 1 }; struct snd_oxfw_stream_formation formation; - unsigned int i, err; + int i, err; for (i = 0; i < SND_OXFW_STREAM_FORMAT_ENTRIES; i++) { if (formats[i] == NULL) @@ -47,7 +47,7 @@ static int hw_rule_channels(struct snd_pcm_hw_params *params, const struct snd_interval *r = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_RATE); struct snd_oxfw_stream_formation formation; - unsigned int i, j, err; + int i, j, err; unsigned int count, list[SND_OXFW_STREAM_FORMAT_ENTRIES] = {0}; count = 0; @@ -80,7 +80,7 @@ static int hw_rule_channels(struct snd_pcm_hw_params *params, static void limit_channels_and_rates(struct snd_pcm_hardware *hw, u8 **formats) { struct snd_oxfw_stream_formation formation; - unsigned int i, err; + int i, err; hw->channels_min = UINT_MAX; hw->channels_max = 0;
This code tends to use unsigned variables by default and it causes signedness bugs when we use negative variables for error handling. The "i" and "j" variables are used to iterated over small positive values and so they should be type "int". The "len" variable doesn't *need* to be signed but it should be signed to make the code easier to read and audit. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>