On Mon, Mar 27, 2017 at 12:54:03PM -0400, Kevin O'Connor wrote:
On Mon, Mar 27, 2017 at 06:31:29PM +0300, Roman Kagan wrote:
On Tue, Mar 14, 2017 at 06:16:04PM +0300, Roman Kagan wrote:
On Mon, Mar 13, 2017 at 12:11:49PM -0400, Kevin O'Connor wrote:
On Thu, Mar 02, 2017 at 07:37:38PM +0300, Roman Kagan wrote:
On Thu, Mar 02, 2017 at 11:14:37AM -0500, Kevin O'Connor wrote:
I have a few high-level comments on the series. I wonder if there is a way to reduce the amount of control passing between the generic scsi code and the driver code. Specifically, I wonder if the callback function scsi_add_lun could be simplified. Some thoughts:
- instead of scsi_rep_luns_scan() being passed a callback, perhaps introduce a scsi_get_luns() command that returns a malloc'd struct containing the list of luns. The driver code could then iterate over the list.
I considered this at first, but it looked like more boilerplate code in the drivers so I decided to go with the callback.
I was thinking the driver could have something like:
struct scsi_luns *luns = scsi_get_luns(&vlun0.drive); for (i = 0; luns && i < luns->count; i++) xxx_add_lun(&vlun0, luns->luns[i]); free(luns);One could layout 'struct scsi_luns' to use the same memory as cdbres_report_luns:
struct scsi_luns { u32 count, pad; u64 luns[0]; }And scsi_get_luns() could fixup the big-endian conversion in-place (or the driver loop above could just directly call be32_to_cpu/be64_to_cpu). If REPORT LUNS fails then scsi_get_luns() could just fill in 'struct scsi_luns' with count=1, lun[0]=0.
I'm not convinced that exposing all this boilerplate is simpler than hiding the details under the hood of a single function. Or is perhaps anything wrong with passing function pointers in general? Anyway I can do it the way you suggest if this is what you want.
- malloc_high shouldn't be used on temporary reservations (as it can fragment the permanent memory pool) - use malloc_tmp instead.
OK
- scsilun2u64() looks like be64_to_cpu - or did I miss something?
No it would've been too simple :) It's a funny mix: the luns are encoded as four be16-s LE-ordered wrt each other.
- I'd prefer to avoid backwards goto's - a "for (;;)" loop would be preferabale
OK
I'll rework and resubmit the patchset once I have your final word on the interface.
Thanks! Roman.