* [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
@ 2018-11-29 1:01 David Disseldorp
2018-11-29 1:15 ` Ly, Bryant
` (5 more replies)
0 siblings, 6 replies; 7+ messages in thread
From: David Disseldorp @ 2018-11-29 1:01 UTC (permalink / raw)
To: target-devel
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 */
--
2.13.7
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
2018-11-29 1:01 [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response David Disseldorp
@ 2018-11-29 1:15 ` Ly, Bryant
2018-11-29 1:30 ` Lee Duncan
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Ly, Bryant @ 2018-11-29 1:15 UTC (permalink / raw)
To: target-devel
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
2018-11-29 1:01 [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response David Disseldorp
2018-11-29 1:15 ` Ly, Bryant
@ 2018-11-29 1:30 ` Lee Duncan
2018-11-29 16:35 ` Bart Van Assche
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Lee Duncan @ 2018-11-29 1:30 UTC (permalink / raw)
To: target-devel
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
2018-11-29 1:01 [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response David Disseldorp
2018-11-29 1:15 ` Ly, Bryant
2018-11-29 1:30 ` Lee Duncan
@ 2018-11-29 16:35 ` Bart Van Assche
2018-11-30 13:17 ` David Disseldorp
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Bart Van Assche @ 2018-11-29 16:35 UTC (permalink / raw)
To: target-devel
On Thu, 2018-11-29 at 02:01 +-0100, David Disseldorp wrote:
+AD4 Use the value stored in t10+AF8-wwn.vendor, which defaults to +ACI-LIO-ORG+ACI, but
+AD4 can be reconfigured via the vendor+AF8-id ConfigFS attribute.
+AD4
+AD4 Signed-off-by: David Disseldorp +ADw-ddiss+AEA-suse.de+AD4
+AD4 ---
+AD4 drivers/target/target+AF8-core+AF8-spc.c +AHw 8 +-+-+-+-+----
+AD4 1 file changed, 5 insertions(+-), 3 deletions(-)
+AD4
+AD4 diff --git a/drivers/target/target+AF8-core+AF8-spc.c b/drivers/target/target+AF8-core+AF8-spc.c
+AD4 index 8ffe712cb44d..4503f3336bc2 100644
+AD4 --- a/drivers/target/target+AF8-core+AF8-spc.c
+AD4 +-+-+- b/drivers/target/target+AF8-core+AF8-spc.c
+AD4 +AEAAQA -115,7 +-115,8 +AEAAQA spc+AF8-emulate+AF8-inquiry+AF8-std(struct se+AF8-cmd +ACo-cmd, unsigned char +ACo-buf)
+AD4 +ACo-/
+AD4 memset(+ACY-buf+AFs-8+AF0, 0x20,
+AD4 INQUIRY+AF8-VENDOR+AF8-LEN +- INQUIRY+AF8-MODEL+AF8-LEN +- INQUIRY+AF8-REVISION+AF8-LEN)+ADs
+AD4 - memcpy(+ACY-buf+AFs-8+AF0, +ACI-LIO-ORG+ACI, sizeof(+ACI-LIO-ORG+ACI) - 1)+ADs
+AD4 +- memcpy(+ACY-buf+AFs-8+AF0, dev-+AD4-t10+AF8-wwn.vendor,
+AD4 +- strnlen(dev-+AD4-t10+AF8-wwn.vendor, INQUIRY+AF8-VENDOR+AF8-LEN))+ADs
+AD4 memcpy(+ACY-buf+AFs-16+AF0, dev-+AD4-t10+AF8-wwn.model,
+AD4 strnlen(dev-+AD4-t10+AF8-wwn.model, INQUIRY+AF8-MODEL+AF8-LEN))+ADs
+AD4 memcpy(+ACY-buf+AFs-32+AF0, dev-+AD4-t10+AF8-wwn.revision,
+AD4 +AEAAQA -258,8 +-259,9 +AEAAQA spc+AF8-emulate+AF8-evpd+AF8-83(struct se+AF8-cmd +ACo-cmd, unsigned char +ACo-buf)
+AD4 buf+AFs-off+-1+AF0 +AD0 0x1+ADs /+ACo T10 Vendor ID +ACo-/
+AD4 buf+AFs-off+-2+AF0 +AD0 0x0+ADs
+AD4 /+ACo left align Vendor ID and pad with spaces +ACo-/
+AD4 - memset(+ACY-buf+AFs-off+-4+AF0, 0x20, 8)+ADs
+AD4 - memcpy(+ACY-buf+AFs-off+-4+AF0, +ACI-LIO-ORG+ACI, sizeof(+ACI-LIO-ORG+ACI) - 1)+ADs
+AD4 +- memset(+ACY-buf+AFs-off+-4+AF0, 0x20, INQUIRY+AF8-VENDOR+AF8-LEN)+ADs
+AD4 +- memcpy(+ACY-buf+AFs-off+-4+AF0, dev-+AD4-t10+AF8-wwn.vendor,
+AD4 +- strnlen(dev-+AD4-t10+AF8-wwn.vendor, INQUIRY+AF8-VENDOR+AF8-LEN))+ADs
+AD4 /+ACo Extra Byte for NULL Terminator +ACo-/
+AD4 id+AF8-len+-+-+ADs
+AD4 /+ACo Identifier Length +ACo-/
Where is the code that initializes dev-+AD4-t10+AF8-wwn.vendor to +ACI-LIO-ORG+ACI? 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
2018-11-29 1:01 [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response David Disseldorp
` (2 preceding siblings ...)
2018-11-29 16:35 ` Bart Van Assche
@ 2018-11-30 13:17 ` David Disseldorp
2018-11-30 14:41 ` David Disseldorp
2018-11-30 16:41 ` Bart Van Assche
5 siblings, 0 replies; 7+ messages in thread
From: David Disseldorp @ 2018-11-30 13:17 UTC (permalink / raw)
To: target-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
2018-11-29 1:01 [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response David Disseldorp
` (3 preceding siblings ...)
2018-11-30 13:17 ` David Disseldorp
@ 2018-11-30 14:41 ` David Disseldorp
2018-11-30 16:41 ` Bart Van Assche
5 siblings, 0 replies; 7+ messages in thread
From: David Disseldorp @ 2018-11-30 14:41 UTC (permalink / raw)
To: target-devel
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@96" > 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
2018-11-29 1:01 [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response David Disseldorp
` (4 preceding siblings ...)
2018-11-30 14:41 ` David Disseldorp
@ 2018-11-30 16:41 ` Bart Van Assche
5 siblings, 0 replies; 7+ messages in thread
From: Bart Van Assche @ 2018-11-30 16:41 UTC (permalink / raw)
To: target-devel
On Fri, 2018-11-30 at 15:41 +-0100, David Disseldorp wrote:
+AD4 On Fri, 30 Nov 2018 14:17:49 +-0100, David Disseldorp wrote:
+AD4
+AD4 +AD4 +AD4 Where is the code that initializes dev-+AD4-t10+AF8-wwn.vendor to +ACI-LIO-ORG+ACI? Did I
+AD4 +AD4 +AD4 perhaps overlook something?
+AD4 +AD4
+AD4 +AD4 This is done in target+AF8-configure+AF8-device() .
+AD4
+AD4 Hmm, continuing to do it there may cause a bit of confusion if vendor+AF8-id
+AD4 is set before the backstore is enabled, which would cause it to be
+AD4 overwritten. That said, we already have similarly strange behaviour for
+AD4 emulate+AF8-model+AF8-alias / product. E.g.:
+AD4
+AD4 rapido1:/+ACM cd /sys/kernel/config/target/
+AD4 rapido1:target+ACM mkdir -p core/fileio+AF8-1/testing
+AD4 rapido1:target+ACM echo -n +ACI-fd+AF8-dev+AF8-name+AD0-/asdf,fd+AF8-dev+AF8-size+AD0-4096+ACI +AD4 core/fileio+AF8-1/testing/control
+AD4 rapido1:target+ACM echo -n +ACI-testing1+ACI +AD4 core/fileio+AF8-1/testing/wwn/vendor+AF8-id
+AD4 rapido1:target+ACM cat core/fileio+AF8-1/testing/wwn/vendor+AF8-id
+AD4 testing1
+AD4 rapido1:target+ACM echo 1 +AD4 ./core/fileio+AF8-1/testing/attrib/emulate+AF8-model+AF8-alias
+AD4 rapido1:target+ACM cat ./core/fileio+AF8-1/testing/statistics/scsi+AF8-lu/prod
+AD4 testing
+AD4 rapido1:target+ACM echo 1 +AD4 core/fileio+AF8-1/testing/enable
+AD4 rapido1:target+ACM cat core/fileio+AF8-1/testing/wwn/vendor+AF8-id
+AD4 LIO-ORG
+AD4 rapido1:target+ACM cat ./core/fileio+AF8-1/testing/statistics/scsi+AF8-lu/prod
+AD4 FILEIO
+AD4 rapido1:target+ACM cat ./core/fileio+AF8-1/testing/attrib/emulate+AF8-model+AF8-alias
+AD4 1
+AD4
+AD4 Not sure how best to handle this...
+AD4 1. move vendor/model/rev initialization into target+AF8-alloc+AF8-device()
+AD4 2. move vendor (only) initialization into target+AF8-alloc+AF8-device()
+AD4 3. fail attempts to set emulate+AF8-model+AF8-alias or vendor+AF8-id before the
+AD4 backstore has been enabled
+AD4 4. leave as-is
+AD4
+AD4 (1) would IMO be the most straightforward, but it's a slight change to
+AD4 the existing (IMO broken) emulate+AF8-model+AF8-alias user interface.
I'm in favor of moving some of the target+AF8-configure+AF8-device() code into the
target+AF8-alloc+AF8-device() function. Today target+AF8-configure+AF8-device() overwrites
the vendor, model and revision string and is called when +ACI-1+ACI is written
into the +ACI-enable+ACI attribute. Overwriting these attributes when enabling a
backstore seems wrong to me.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-11-30 16:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-29 1:01 [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response David Disseldorp
2018-11-29 1:15 ` Ly, Bryant
2018-11-29 1:30 ` Lee Duncan
2018-11-29 16:35 ` Bart Van Assche
2018-11-30 13:17 ` David Disseldorp
2018-11-30 14:41 ` David Disseldorp
2018-11-30 16:41 ` Bart Van Assche
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.