On Tue, Mar 28, 2017 at 07:34:32PM +0300, Roman Kagan wrote:
On Mon, Mar 27, 2017 at 12:54:03PM -0400, Kevin O'Connor wrote:
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.
Function pointers are a bit funky in SeaBIOS - one can't mix them between 16bit and 32bit mode and one has to be careful with function addresses referenced in init only code that are used outside of init only code. I don't think either of the above is an issue in this case.
- 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.
Okay - thanks.
- 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.
The issues above on patch 2 I think do need to be addressed (excluding my misunderstanding of scsilun2u64). The use of function pointers is not a show stopper, I just suspect it would make the code a little easier to follow.
Thanks, -Kevin