| Message ID | 20181129010157.12687-7-ddiss@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target: user configurable T10 Vendor ID | expand |
> 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
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>
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.
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
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
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 --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 */
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(-)