Hi Bart,
> On Jun 7, 2017, at 3:45 PM, Bart Van Assche <bart.vanassche@xxxxxxxxxxx> wrote:
>
> On Wed, 2017-06-07 at 14:43 -0700, Himanshu Madhani wrote:
>> +enum {
>> + TYPE_SRB,
>> + TYPE_TGT_CMD,
>> +};
>> +
>> typedef struct srb {
>> + /*
>> + * Do not move cmd_type field, it needs to
>> + * line up with qla_tgt_cmd->cmd_type
>> + */
>> + uint8_t cmd_type;
>> + uint8_t pad[3];
>> atomic_t ref_count;
>> struct fc_port *fcport;
>> struct scsi_qla_host *vha;
>
> [ ... ]
>
>> struct qla_tgt_cmd {
>> + /*
>> + * Do not move cmd_type field. it needs to line up with srb->cmd_type
>> + */
>> + uint8_t cmd_type;
>> + uint8_t pad[7];
>> struct se_cmd se_cmd;
>> struct fc_port *sess;
>> int state;
>
> Hello Quinn and Himanshu,
>
> Sorry but this is really inelegant. Have you considered the following?
> - Keep the existing srb and qla_tgt_cmd data structures.
> - Introduce a new data structure with enum cmd_type as the first member and
> a union of struct srb and struct qla_tgt_cmd as the second member.
> - Use __attribute__((aligned(...))) to express alignment requirements instead
> of explicitly inserting padding bytes.
>
> With that approach no casts are needed to convert a pointer to the new data
> structure into a struct srb or struct qla_tgt_cmd pointer - all that will be
> needed is to take the address of the appropriate member of the union.
>
> Thanks,
>
> Bart.
We are working on addressing this review comment. We’ll send it as a separate
patch once we run through our regression test cycle.
Thanks,
- Himanshu
