Re: [PATCH] ksmbd: clean up a type in ksmbd_vfs_stream_write()

[Dan Carpenter <dan.carpenter@oracle.com>]

On Fri, Jun 03, 2022 at 09:45:21AM +0900, Namjae Jeon wrote:
> 2022-06-03 9:10 GMT+09:00, Dan Carpenter <dan.carpenter@oracle.com>:
> > On Fri, Jun 03, 2022 at 08:18:19AM +0900, Namjae Jeon wrote:
> >> 2022-06-02 19:51 GMT+09:00, Dan Carpenter <dan.carpenter@oracle.com>:
> >> > @@ -428,9 +429,9 @@ static int ksmbd_vfs_stream_write(struct ksmbd_file
> >> > *fp,
> >> > char *buf, loff_t *pos,
> >> >  				       fp->stream.name,
> >> >  				       fp->stream.size,
> >> >  				       &stream_buf);
> >> > -	if ((int)v_len < 0) {
> >> > +	if (v_len < 0) {
> >> >  		pr_err("not found stream in xattr : %zd\n", v_len);
> >> > -		err = (int)v_len;
> >> > +		err = v_len;
> >> Data type of ssize_t is long. Wouldn't some static checker warn us
> >> that this is a problem?
> >
> > None that I know of.
> >
> > To a human reader, the cast isn't needed because when a function returns
> > ssize_t the negatives can only be error codes in the (-4095)-(-1) range.
> > No other negative sizes make sense.
> >
> > On the other hand, the "if ((int)v_len < 0) {" line really should
> > trigger a static checker because there are times when sizes could be
> > over 2GB.  I will write down that I need to create that checker.
> Okay, And there is a similar type casting in ksmbd_vfs_stream_read().
> Is it not necessary to change together?

The motivation for this change was that I was looking for places which
save negative error codes in a positive value.  Since the ksmbd_vfs_stream_read()
is using ssize_t then it didn't trigger that warning.  But
ksmbd_vfs_stream_read() has some minor issues.

	if ((int)v_len <= 0)
This cast is "buggy".  Harmless though.

		return (int)v_len;
This is fine, but unnecessary.

	if (v_len <= *pos) {
		count = -EINVAL;

I feel like this should return 0 bytes instead of -EINVAL;  It probably
doesn't matter.

I am going to try write a static checker that complains specifically
about casting size_t (not long) to int.  I will make it complain about
ssize_t as well, but that's kind of unusual because ssize_t can already
store negatives.

regards,
dan carpenter