diff mbox series

[v4,6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response

Message ID 20181129010157.12687-7-ddiss@suse.de (mailing list archive)
State Superseded
Headers show
Series target: user configurable T10 Vendor ID | expand

Commit Message

David Disseldorp Nov. 29, 2018, 1:01 a.m. UTC
Use the value stored in t10_wwn.vendor, which defaults to "LIO-ORG", but
can be reconfigured via the vendor_id ConfigFS attribute.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 drivers/target/target_core_spc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Ly, Bryant Nov. 29, 2018, 1:15 a.m. UTC | #1
> On Nov 28, 2018, at 7:01 PM, David Disseldorp <ddiss@suse.de> wrote:
> 
> Use the value stored in t10_wwn.vendor, which defaults to "LIO-ORG", but
> can be reconfigured via the vendor_id ConfigFS attribute.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
> drivers/target/target_core_spc.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
> 

Reviewed-by: Bryant G. Ly bly@catalogicsoftware.com
Lee Duncan Nov. 29, 2018, 1:30 a.m. UTC | #2
On 11/28/18 5:01 PM, David Disseldorp wrote:
> Use the value stored in t10_wwn.vendor, which defaults to "LIO-ORG", but
> can be reconfigured via the vendor_id ConfigFS attribute.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  drivers/target/target_core_spc.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
> index 8ffe712cb44d..4503f3336bc2 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -115,7 +115,8 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char *buf)
>  	 */
>  	memset(&buf[8], 0x20,
>  	       INQUIRY_VENDOR_LEN + INQUIRY_MODEL_LEN + INQUIRY_REVISION_LEN);
> -	memcpy(&buf[8], "LIO-ORG", sizeof("LIO-ORG") - 1);
> +	memcpy(&buf[8], dev->t10_wwn.vendor,
> +	       strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
>  	memcpy(&buf[16], dev->t10_wwn.model,
>  	       strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
>  	memcpy(&buf[32], dev->t10_wwn.revision,
> @@ -258,8 +259,9 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char *buf)
>  	buf[off+1] = 0x1; /* T10 Vendor ID */
>  	buf[off+2] = 0x0;
>  	/* left align Vendor ID and pad with spaces */
> -	memset(&buf[off+4], 0x20, 8);
> -	memcpy(&buf[off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
> +	memset(&buf[off+4], 0x20, INQUIRY_VENDOR_LEN);
> +	memcpy(&buf[off+4], dev->t10_wwn.vendor,
> +	       strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
>  	/* Extra Byte for NULL Terminator */
>  	id_len++;
>  	/* Identifier Length */
> 


Reviewed-by: Lee Duncan <lduncan@suse.com>
Bart Van Assche Nov. 29, 2018, 4:35 p.m. UTC | #3
On Thu, 2018-11-29 at 02:01 +0100, David Disseldorp wrote:
> Use the value stored in t10_wwn.vendor, which defaults to "LIO-ORG", but
> can be reconfigured via the vendor_id ConfigFS attribute.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  drivers/target/target_core_spc.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
> index 8ffe712cb44d..4503f3336bc2 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -115,7 +115,8 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char *buf)
>  	 */
>  	memset(&buf[8], 0x20,
>  	       INQUIRY_VENDOR_LEN + INQUIRY_MODEL_LEN + INQUIRY_REVISION_LEN);
> -	memcpy(&buf[8], "LIO-ORG", sizeof("LIO-ORG") - 1);
> +	memcpy(&buf[8], dev->t10_wwn.vendor,
> +	       strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
>  	memcpy(&buf[16], dev->t10_wwn.model,
>  	       strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
>  	memcpy(&buf[32], dev->t10_wwn.revision,
> @@ -258,8 +259,9 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char *buf)
>  	buf[off+1] = 0x1; /* T10 Vendor ID */
>  	buf[off+2] = 0x0;
>  	/* left align Vendor ID and pad with spaces */
> -	memset(&buf[off+4], 0x20, 8);
> -	memcpy(&buf[off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
> +	memset(&buf[off+4], 0x20, INQUIRY_VENDOR_LEN);
> +	memcpy(&buf[off+4], dev->t10_wwn.vendor,
> +	       strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
>  	/* Extra Byte for NULL Terminator */
>  	id_len++;
>  	/* Identifier Length */

Where is the code that initializes dev->t10_wwn.vendor to "LIO-ORG"? Did I
perhaps overlook something? Additionally, why are you using strnlen() for
a string of which it is guaranteed that its length is less than or equal to
the second strnlen() argument?

Thanks,

Bart.
David Disseldorp Nov. 30, 2018, 1:17 p.m. UTC | #4
On Thu, 29 Nov 2018 08:35:26 -0800, Bart Van Assche wrote:

> Where is the code that initializes dev->t10_wwn.vendor to "LIO-ORG"? Did I
> perhaps overlook something?

This is done in target_configure_device() .

> Additionally, why are you using strnlen() for
> a string of which it is guaranteed that its length is less than or equal to
> the second strnlen() argument?

Belt and braces :) Actually, I think it's a little more readable this
way.

Cheers, David
David Disseldorp Nov. 30, 2018, 2:41 p.m. UTC | #5
On Fri, 30 Nov 2018 14:17:49 +0100, David Disseldorp wrote:

> > Where is the code that initializes dev->t10_wwn.vendor to "LIO-ORG"? Did I
> > perhaps overlook something?  
> 
> This is done in target_configure_device() .

Hmm, continuing to do it there may cause a bit of confusion if vendor_id
is set before the backstore is enabled, which would cause it to be
overwritten. That said, we already have similarly strange behaviour for
emulate_model_alias / product. E.g.:

rapido1:/# cd /sys/kernel/config/target/
rapido1:target# mkdir -p core/fileio_1/testing
rapido1:target# echo -n "fd_dev_name=/asdf,fd_dev_size=4096" > core/fileio_1/testing/control
rapido1:target# echo -n "testing1" > core/fileio_1/testing/wwn/vendor_id
rapido1:target# cat core/fileio_1/testing/wwn/vendor_id
testing1
rapido1:target# echo 1 > ./core/fileio_1/testing/attrib/emulate_model_alias
rapido1:target# cat ./core/fileio_1/testing/statistics/scsi_lu/prod
testing
rapido1:target# echo 1 > core/fileio_1/testing/enable
rapido1:target# cat core/fileio_1/testing/wwn/vendor_id
LIO-ORG
rapido1:target# cat ./core/fileio_1/testing/statistics/scsi_lu/prod
FILEIO
rapido1:target# cat ./core/fileio_1/testing/attrib/emulate_model_alias
1

Not sure how best to handle this...
1. move vendor/model/rev initialization into target_alloc_device()
2. move vendor (only) initialization into target_alloc_device()
3. fail attempts to set emulate_model_alias or vendor_id before the
   backstore has been enabled
4. leave as-is

(1) would IMO be the most straightforward, but it's a slight change to
the existing (IMO broken) emulate_model_alias user interface.

Cheers, David
Bart Van Assche Nov. 30, 2018, 4:41 p.m. UTC | #6
On Fri, 2018-11-30 at 15:41 +0100, David Disseldorp wrote:
> On Fri, 30 Nov 2018 14:17:49 +0100, David Disseldorp wrote:
> 
> > > Where is the code that initializes dev->t10_wwn.vendor to "LIO-ORG"? Did I
> > > perhaps overlook something?  
> > 
> > This is done in target_configure_device() .
> 
> Hmm, continuing to do it there may cause a bit of confusion if vendor_id
> is set before the backstore is enabled, which would cause it to be
> overwritten. That said, we already have similarly strange behaviour for
> emulate_model_alias / product. E.g.:
> 
> rapido1:/# cd /sys/kernel/config/target/
> rapido1:target# mkdir -p core/fileio_1/testing
> rapido1:target# echo -n "fd_dev_name=/asdf,fd_dev_size=4096" > core/fileio_1/testing/control
> rapido1:target# echo -n "testing1" > core/fileio_1/testing/wwn/vendor_id
> rapido1:target# cat core/fileio_1/testing/wwn/vendor_id
> testing1
> rapido1:target# echo 1 > ./core/fileio_1/testing/attrib/emulate_model_alias
> rapido1:target# cat ./core/fileio_1/testing/statistics/scsi_lu/prod
> testing
> rapido1:target# echo 1 > core/fileio_1/testing/enable
> rapido1:target# cat core/fileio_1/testing/wwn/vendor_id
> LIO-ORG
> rapido1:target# cat ./core/fileio_1/testing/statistics/scsi_lu/prod
> FILEIO
> rapido1:target# cat ./core/fileio_1/testing/attrib/emulate_model_alias
> 1
> 
> Not sure how best to handle this...
> 1. move vendor/model/rev initialization into target_alloc_device()
> 2. move vendor (only) initialization into target_alloc_device()
> 3. fail attempts to set emulate_model_alias or vendor_id before the
>    backstore has been enabled
> 4. leave as-is
> 
> (1) would IMO be the most straightforward, but it's a slight change to
> the existing (IMO broken) emulate_model_alias user interface.

I'm in favor of moving some of the target_configure_device() code into the
target_alloc_device() function. Today target_configure_device() overwrites
the vendor, model and revision string and is called when "1" is written
into the "enable" attribute. Overwriting these attributes when enabling a
backstore seems wrong to me.

Thanks,

Bart.
diff mbox series

Patch

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index 8ffe712cb44d..4503f3336bc2 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -115,7 +115,8 @@  spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char *buf)
 	 */
 	memset(&buf[8], 0x20,
 	       INQUIRY_VENDOR_LEN + INQUIRY_MODEL_LEN + INQUIRY_REVISION_LEN);
-	memcpy(&buf[8], "LIO-ORG", sizeof("LIO-ORG") - 1);
+	memcpy(&buf[8], dev->t10_wwn.vendor,
+	       strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
 	memcpy(&buf[16], dev->t10_wwn.model,
 	       strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
 	memcpy(&buf[32], dev->t10_wwn.revision,
@@ -258,8 +259,9 @@  spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char *buf)
 	buf[off+1] = 0x1; /* T10 Vendor ID */
 	buf[off+2] = 0x0;
 	/* left align Vendor ID and pad with spaces */
-	memset(&buf[off+4], 0x20, 8);
-	memcpy(&buf[off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
+	memset(&buf[off+4], 0x20, INQUIRY_VENDOR_LEN);
+	memcpy(&buf[off+4], dev->t10_wwn.vendor,
+	       strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
 	/* Extra Byte for NULL Terminator */
 	id_len++;
 	/* Identifier Length */