[Stgt-devel] [Patch 1/2] Add-new-module-entry-points-for-configuration - Try 5

[FUJITA Tomonori]

From: "Mark Harvey" <markh794 at gmail.com>
Subject: [Stgt-devel] [Patch 1/2]	Add-new-module-entry-points-for-configuration - Try 5
Date: Tue, 5 Jun 2007 13:18:04 +1000

> OK, just when I thought I'd found all my coding blunders...
> 
> A quick 'grep "if\(" identified some of my mistakes and a couple of
> other that slipped thru.

Applied, thanks a lot. No base64 attachment next time, please.

Some comments for next time.

>  int spc_inquiry(int host_no, struct scsi_cmd *cmd)
>  {
> @@ -40,7 +44,7 @@ int spc_inquiry(int host_no, struct scsi_cmd *cmd)
>  	uint8_t *data;
>  	uint8_t *scb = cmd->scb;
>  	unsigned char device_type = cmd->c_target->dev_type_template.type;
> -	char *product_id = cmd->c_target->dev_type_template.pid;
> +	struct lu_phy_attr *attributes = cmd->dev->attributes;
>  	unsigned char key = ILLEGAL_REQUEST, asc = 0x24;
>  
>  	if (((scb[1] & 0x3) == 0x3) || (!(scb[1] & 0x3) && scb[2]))
> @@ -57,22 +61,21 @@ int spc_inquiry(int host_no, struct scsi_cmd *cmd)
>  	dprintf("%x %x\n", scb[1], scb[2]);
>  
>  	if (!(scb[1] & 0x3)) {
> +		int i;
>  		data[0] = device_type;
> -		data[2] = 4;
> +		data[1] = (attributes->is_removable) ? 0x80 : 0;
> +		data[2] = 5;	/* SPC-3 */
>  		data[3] = 0x42;
> -		data[4] = 59;
>  		data[7] = 0x02;
>  		memset(data + 8, 0x20, 28);
> -		strncpy((char *)data + 8, VENDOR_ID, 8);
> -		strncpy((char *)data + 16, product_id, 16);
> -		strncpy((char *)data + 32, PRODUCT_REV, 4);
> -		data[58] = 0x03;
> -		data[59] = 0x20;
> -		data[60] = 0x09;
> -		data[61] = 0x60;
> -		data[62] = 0x03;
> -		data[63] = 0x00;
> -		len = 64;
> +		strncpy((char *)data + 8, attributes->vendor_ident, 8);
> +		strncpy((char *)data + 16, attributes->product_ident, 16);
> +		strncpy((char *)data + 32, attributes->product_rev, 4);
> +		for (i=0, len=58; i < VERSION_DESCRIPTOR_LEN; i++, len += 2) {
> +			data[len] = (attributes->version_desc[i] >> 8) & 0xff;
> +			data[len+1] = attributes->version_desc[i] & 0xff;
> +		}

Should be:

> +		for (i = 0, len = 58; i < VERSION_DESCRIPTOR_LEN; i++, len += 2) {

Anyway, this part looks odd to me. I rewrote this part.


> +struct lu_phy_attr {
> +	char scsi_id[SCSI_ID_LEN];
> +	char scsi_sn[SCSI_SN_LEN];
> +
> +	/* SCSI Inquiry Params */
> +	char vendor_ident[9];
> +	char product_ident[17];

Renamed vendor_id and product_id.

> +	char product_rev[5];
> +	uint16_t version_desc[VERSION_DESCRIPTOR_LEN];
> +
> +	char is_removable;	/* Removable media */
> +	char online;		/* Logical Unit online ? */
> +	char reset;		/* Power-on or reset has occured */
> +	char sense_format;	/* Descrptor format sense data supported */
> +};
> +
>  struct scsi_lu {
>  	int fd;
>  	uint64_t addr; /* persistent mapped address */
>  	uint64_t size;
>  	uint64_t lun;
> -	char scsi_id[SCSI_ID_LEN];
> -	char scsi_sn[SCSI_SN_LEN];
>  	char *path;
>  
>  	/* the list of devices belonging to a target */
> @@ -48,7 +63,7 @@ struct scsi_lu {
>  	uint64_t reserve_id;
>  
>  	/* TODO: needs a structure for lots of device parameters */
> -	uint8_t d_sense;
> +	struct lu_phy_attr *attributes;

Why do we use a struct lu_phy_attr pointer? Why just can't we do:

struct scsi_lu {
...
	struct lu_phy_attr attrs;
};

'attributes' is too long for me. renamed 'attrs'.