On Sun, Jun 27, 2021 at 08:14:48PM +0300, i.kononenko wrote: > > > On 27.06.2021 17:23, Alan Stern wrote: > > On Sun, Jun 27, 2021 at 12:18:15AM +0300, Igor Kononenko wrote: > >> Implements a universal way to define SCSI commands and configure > >> precheck handlers. > > > > What is the reason for doing this? > > I have started implementing a way to specify a backend-file of > mass-storage images greater than 2.1Gb for cdrom-like mediums. > I notice the implementation of each scsi-command handler uses too > many magic-constant, hardcoded indexes and shifts. I decided to > define structures that contained appropriate SCSI-defined fields > and constant-values to clarify the code. > > Additionally, I noticed, many kernel subsystems use the 'separate > data and logic' approach, making a code more explicit and readable. > This looks reasonable to me, and a code looks more clearly, at > least - we don't need to examine each magic constant and its purpose. > > > > > At first glance, it appears you have added a great deal of complexity > > to the driver. The patch replaces a large amount of easily understood > > (albeit rather repetitious) code with an approximately equal amount > > of rather complicated code. This does not seem like an improvement. > > The SCSI-commands table is defined as unifying a way to specify the > SCSI-command handler, with corresponding required data instead pass > it to each repeatedly switch-case block, which makes code more readable > to me. If there isn't, I can keep the definition of SCSI-handlers as is, > but the SCSI-data structures with their constant-values are still > required, in my opinion. > > > > > Furthermore, the code you removed is flexible; it easily allows for > > small variations as neede by some command handlers. But the code you > > added is all table-driven, which does not easily permit arbitrary > > variations. > > > > I don't think that the SCSI-command handlers table is an obstacle to > define variation into a specific handler because the current patch has > helper macros, which can specify a behavior for each requirement of > handler. > > Anyway, the definition of the scsi-command handlers table may be discarded, > because this work done to helping developers who will work the > 'usb:gadget:mass-storage' subsystem in the future. Can you submit a patch that adds only the data structures without the commands table? Alan Stern
