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

[Bart Van Assche]

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.

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

[David Disseldorp]

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

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

[David Disseldorp]

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

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

[Bart Van Assche]

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 
> ---
>  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.

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

[Lee Duncan]

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 
> ---
>  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 

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

[Ly, Bryant]

> On Nov 28, 2018, at 7: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 
> ---
> drivers/target/target_core_spc.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
> 

Reviewed-by: Bryant G. Ly b...@catalogicsoftware.com

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

[David Disseldorp]

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 
---
 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 */
-- 
2.13.7