David Somayajulu wrote:
However I have one minor gripe, which is regarding the vendor unique id in the receive path. This makes it appear as if the scsi netlink layer is peeking into its payload (where each msg is treated as <hdr -containing address information><payload>). In the network stack, each layer looks at the packet it receives and consumes if it is the final destination, otherwise determines the next destination from the header and sends it. No layer peeks into the payload if it is not the consumer. The only integrity check is typically a checksum.
I understand your intent - but view it a little differently. I break the "stack layers" into the following: - "scsi netlink", which is the glue logic for our SCSI channel, that everything use. It does the first level of message decode (consuming scsi_nl_hdr), performing the main validity checks (length vs netlink packet), and selecting a transport. - "transport", which could be the SAS transport, FC transport, or the catch-all SCSI (midlayer) transport. Each transport has its own message types. It may fully handle the message, or can be a multiplexer for the components that belong to it. - The component layer - with shosts belonging to the catch-all SCSI transport. FC components would be fc_host's, fc_rports, etc; and so-on for SAS.
In this case, the vendor shost msg maps to the catch-all SCSI transport. The SCSI transport consumes scsi_nl_host_vendor_msg, performs common steps such as driver validation, then pushes the payload to LLDD/shost for the rest of the processing. So same concepts as you describe.
There is only one violation of the "don't peek into the next layer" rule, but it's around scsi_nl_header:msgtype (not vendor unique id). The field belongs proper to the transport, but is contained in the glue header.
I agree the vendor unique id initially feels odd. We needed some handle to recognize the driver entry points by for registration/deregistration, and we also needed at least 1 validation of something to ensure the driver and the message match. Given that we had it in the event messages, I used it as the handle. I couldn't think of a better alternative.
> I would suggest that we augment the " struct
scsi_nl_hdr" with a bunch of reserved bytes and which are interpreted based on the scsi_nl_hdr->msgtype values, or have "struct scsi_nl_host_vendor_msg" sans "struct scsi_nl_hdr" as part of a union within "struct scsi_nl_hdr" which is interpreted based on scsi_nl_hdr->msgtype values. In all this is a minor issue which is more about details and doesn't matter either way.
I don't see what this buys... Unless you are just expanding the message headers for some future binary-compatible expansion.
-- james s -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
