Re: [CHECKER] Implementation inconsistencies in 2.6.3

[Matthew Dharm]

On Tue, Apr 27, 2004 at 03:58:33PM -0700, Ken Ashcraft wrote:
> > On Tue, Apr 27, 2004 at 02:47:22PM -0700, Ken Ashcraft wrote:
> >
> >> ---------------------------------------------------------
> >> [BUG] (mdharm-usb@xxxxxxxxxxxxxxxxxx) looks like it should return count
> >> instead of strlen(buf), but this is in scsiglue.c, so is it special
> >> code?
> >>
> >> example:
> >> /home/kash/linux/2.6.3/linux-2.6.3/drivers/scsi/scsi_sysfs.c:274:store_rescan_field:
> >> NOTE:READ: Checking arg count [EXAMPLE=device_attribute.store-2]
> >>
> >> /home/kash/linux/2.6.3/linux-2.6.3/drivers/usb/storage/scsiglue.c:321:store_max_sectors:
> >> ERROR:READ: Not checking arg [COUNTER=device_attribute.store-2]  [fit=1]
> >> [fit_fn=1] [fn_ex=0] [fn_counter=1] [ex=233] [counter=1] [z >
> >> 3.20943839741638] [fn-z = -4.35889894354067]
> >>
> >> 	return sprintf(buf, "%u\n", sdev->request_queue->max_sectors);
> >> }
> >>
> >> /* Input routine for the sysfs max_sectors file */
> >>
> >> Error --->
> >> static ssize_t store_max_sectors(struct device *dev, const char *buf,
> >> 		size_t count)
> >> {
> >> 	struct scsi_device *sdev = to_scsi_device(dev);
> >
> > My understanding was that I was supposed to return the number of bytes in
> > the buffer that I actually used.  I thought 'count' was the maximum size I
> > could use.
> >
> 
> That sounds feasible.  I can't find any documentation on the interface, so
> I can't tell.  However, there are ~233 functions that at least reference
> count.  Many of them are almost identical: they call sscanf or strtol to
> get a length out of buf, pass that length to some subroutine, and then
> unconditionally return count.  This is a more representative example than
> the one listed above.
> 
> static ssize_t set_pwm_enable1(struct device *dev, const char *buf,
>                                 size_t count)
> {
>         struct i2c_client *client = to_i2c_client(dev);
>         struct asb100_data *data = i2c_get_clientdata(client);
>         unsigned long val = simple_strtoul(buf, NULL, 10);
>         data->pwm &= 0x0f; /* keep the duty cycle bits */
>         data->pwm |= (val ? 0x80 : 0x00);
>         asb100_write_value(client, ASB100_REG_PWM1, data->pwm);
>         return count;
> }

It seems pretty shady to me to have a function which is passed a value, the
entire purpose of which is to return it to the caller unchanged.

Perhaps someone on l-k can comment...

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@xxxxxxxxxxxxxxxxxx 
Maintainer, Linux USB Mass Storage Driver

Hey Chief.  We've figured out how to save the technical department.  We 
need to be committed.
					-- The Techs
User Friendly, 1/22/1998

Attachment: pgp00000.pgp
Description: PGP signature