On Sun, Jun 27, 2021 at 06:32:03PM +0300, i.kononenko wrote:
> Good morning, Alan!
>
> First of all, thank you for your time to review my first patchset for
> the Linux Kernel and valuable advice on the right way of patchwriting!
>
> On 27.06.2021 17:18, Alan Stern wrote:
> > On Sun, Jun 27, 2021 at 12:18:14AM +0300, Igor Kononenko wrote:
> >> SCSI command handlers currently have an ambiguous return value. This
> >
> > (I dislike very much this way of writing patch descriptions. Unless
> > the reader has already looked at the email subject line and remembers
> > that this patch affects the mass-storage gadget, he will think the
> > sentence above is talking about command handlers in the SCSI core -- a
> > completely different part of the kernel. When writing patch
> > descriptions, please do not assume that the reader already knows what
> > the patch is about.)
> >
> >> return value may indicate the length of the data written to the response
> >> buffer and the command's processing status. Thus, the understanding of
> >> command handling may be implicit.
>
> First of all, thank you for your time to review my first patchset for the
> Linux Kernel and valuable advice on the right way of patchwriting!
>
> I noticed that the status/datasize return value pattern is pervasive for
> Linux and used through many subsystems. But for the f_mass_storage.c,
> such approach use case is not documented anywhere, and implementation has
> too many magic-constant, e.g.
> ```
> static int do_inquiry(struct fsg_common *common, struct fsg_buffhd *bh)
> {
> ....
> return 36;
> }
> ```
> IMHO, this way is not giving the developer an explicit understanding of
> 'what is the 36' and its origin.
> If moving to the suggested way is unwanted, I'd keep the implementation
> as is with additional documentation for each function where uses this
> approach.
Since every one of the command handler functions uses this convention,
it would be wasteful to have separate documentation of the return value
for each function. A single documentation comment that covers all the
command handlers would be acceptable.
> Additionally, I guess, define clarify macros of return value instead of
> magic numbers is required.
If you want, okay. That should go in a separate patch from the
documentation patch.
Also, since the return values are different for each command handler, I
suggest that the macro definitions be placed along with the handler
functions and not in a separate header file. Having a separate file for
these macros would not make any sense, because the values do not need to
be shared across multiple functions or source files.
> > The return value is _not_ ambiguous. If the value is >= 0 then it is
> > a data length, otherwise it is a status. Yes, this is implicit, but it
> > is a very common pattern used throughout the kernel and everyone
> > understands it.
> >
> >> After this patch, the output buffer's size will be set in the
> >> 'data_size_to_handle' field of 'struct fsg_common', and the command
> >> handler's return value indicates only the processing status.
> >
> > What is the reason for making this change? Does it fix any problems
> > or prepare the way for any future patches? It seems like this is
> > completely unnecessary.
>
> Yes, the patch uses as part of the incoming implementation of refactoring
> 'usb:gadget:mass-storage:scsi' command handling.
That incoming implementation uses the refactored command handling but
doesn't depend on the refactoring. It could just as easily use the
existing command handling.
> I believed the suggested improvement would be useful for the community as
> an improvement of code.
Unless you can provide a convincing reason for this change, it doesn't
seem like an improvement to me. It's no easier to read or understand,
and it doesn't improve execution speed on a critical pathway. It just
seems like pointless code churn.
Alan Stern
