diff mbox series

[v6,3/5] target: add device vendor_id configfs attribute

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

Commit Message

David Disseldorp Dec. 4, 2018, 10:12 a.m. UTC 
The vendor_id attribute will allow for the modification of the T10
Vendor Identification string returned in inquiry responses. Its value
can be viewed and modified via the ConfigFS path at:
target/core/$backstore/$name/wwn/vendor_id

"LIO-ORG" remains the default value, which is set when the backstore
device is enabled.

Signed-off-by: David Disseldorp <ddiss@suse.de>
Reviewed-by: Bryant G. Ly <bly@catalogicsoftware.com>
Reviewed-by: Lee Duncan <lduncan@suse.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
---
 drivers/target/target_core_configfs.c | 48 +++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

Comments

Roman Bolshakov Dec. 4, 2018, 12:13 p.m. UTC | #1 
On Tue, Dec 04, 2018 at 11:12:36AM +0100, David Disseldorp wrote:
> The vendor_id attribute will allow for the modification of the T10
> Vendor Identification string returned in inquiry responses. Its value
> can be viewed and modified via the ConfigFS path at:
> target/core/$backstore/$name/wwn/vendor_id
> 
> "LIO-ORG" remains the default value, which is set when the backstore
> device is enabled.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> Reviewed-by: Bryant G. Ly <bly@catalogicsoftware.com>
> Reviewed-by: Lee Duncan <lduncan@suse.com>
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/target/target_core_configfs.c | 48 +++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index 34872f24e8bf..67303c3f9cb4 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> +
> +	/* Assume ASCII encoding. Strip any newline added from userspace. */
> +	BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
> +	strlcpy(dev->t10_wwn.vendor, strstrip(buf),
> +		sizeof(dev->t10_wwn.vendor));
> +

Should we allow non-ASCII characters? It's okay to strip final newline
for convenience. A simple loop would ensure the rest is conformant to
SPC. EINVAL can be returned otherwise.

And for fuzzy purposes there could be a special backstore that does all
sorts of nasty things.

According to SPC 4.3.1:
ASCII data fields shall contain only ASCII printable characters (i.e.,
code values 20h to 7Eh) and may be terminated with one or more ASCII
null (00h) characters.

3.3.10 shall
keyword indicating a mandatory requirement
Note 1 to entry: Designers are required to implement all such
interoperability with other products that conform to this standard.

Thank you,
Roman
Roman Bolshakov Dec. 4, 2018, 1:26 p.m. UTC | #2 
On Tue, Dec 04, 2018 at 03:13:51PM +0300, Roman Bolshakov wrote:
> On Tue, Dec 04, 2018 at 11:12:36AM +0100, David Disseldorp wrote:
> > The vendor_id attribute will allow for the modification of the T10
> > Vendor Identification string returned in inquiry responses. Its value
> > can be viewed and modified via the ConfigFS path at:
> > target/core/$backstore/$name/wwn/vendor_id
> > 
> > "LIO-ORG" remains the default value, which is set when the backstore
> > device is enabled.
> > 
> > Signed-off-by: David Disseldorp <ddiss@suse.de>
> > Reviewed-by: Bryant G. Ly <bly@catalogicsoftware.com>
> > Reviewed-by: Lee Duncan <lduncan@suse.com>
> > Reviewed-by: Hannes Reinecke <hare@suse.com>
> > ---
> >  drivers/target/target_core_configfs.c | 48 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> > 
> > diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> > index 34872f24e8bf..67303c3f9cb4 100644
> > --- a/drivers/target/target_core_configfs.c
> > +++ b/drivers/target/target_core_configfs.c
> > +
> > +	/* Assume ASCII encoding. Strip any newline added from userspace. */
> > +	BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
> > +	strlcpy(dev->t10_wwn.vendor, strstrip(buf),
> > +		sizeof(dev->t10_wwn.vendor));
> > +
> 
> Should we allow non-ASCII characters? It's okay to strip final newline
> for convenience. A simple loop would ensure the rest is conformant to
> SPC. EINVAL can be returned otherwise.
> 
> And for fuzzy purposes there could be a special backstore that does all
> sorts of nasty things.
> 
> According to SPC 4.3.1:
> ASCII data fields shall contain only ASCII printable characters (i.e.,
> code values 20h to 7Eh) and may be terminated with one or more ASCII
> null (00h) characters.
> 
> 3.3.10 shall
> keyword indicating a mandatory requirement
> Note 1 to entry: Designers are required to implement all such
> interoperability with other products that conform to this standard.
> 
> Thank you,
> Roman

wrt PATCH 5 in the series. Should we allow to set vendor_id for for
pscsi? target_transport_configure sets t10_wwn fields for pscsi, but but
an attempt to set vendor_id will overwrite the value recieved from
scsi_device.

And (please correct me if I'm wrong) it's not used anywhere except
statistics config_group. That means in the actual INQUIRY commands it
will still return vendor_id of the underlying storage. If that's that's
true, this means an attempt to set vendor_id will be succesful but it
won't do what's intended for.

Perhaps -ENOSYS or an error code of your preference could be returned if
device has TRANSPORT_FLAG_PASSTHROUGH.

Roman
David Disseldorp Dec. 4, 2018, 2:31 p.m. UTC | #3 
On Tue, 4 Dec 2018 16:26:59 +0300, Roman Bolshakov wrote:

> wrt PATCH 5 in the series. Should we allow to set vendor_id for for
> pscsi? target_transport_configure sets t10_wwn fields for pscsi, but but
> an attempt to set vendor_id will overwrite the value recieved from
> scsi_device.

I considered adding an if (PASSTHROUGH){return -EINVAL}, but we already
allow the model/product string to be set for pscsi+tcmu backends via
emulate_model_alias, so decided against it.

> And (please correct me if I'm wrong) it's not used anywhere except
> statistics config_group. That means in the actual INQUIRY commands it
> will still return vendor_id of the underlying storage. If that's that's
> true, this means an attempt to set vendor_id will be succesful but it
> won't do what's intended for.

That's correct.

Cheers, David
David Disseldorp Dec. 4, 2018, 2:58 p.m. UTC | #4 
On Tue, 4 Dec 2018 15:13:51 +0300, Roman Bolshakov wrote:

> > +	/* Assume ASCII encoding. Strip any newline added from userspace. */
> > +	BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
> > +	strlcpy(dev->t10_wwn.vendor, strstrip(buf),
> > +		sizeof(dev->t10_wwn.vendor));
> > +  
> 
> Should we allow non-ASCII characters?

No :)

> It's okay to strip final newline
> for convenience. A simple loop would ensure the rest is conformant to
> SPC. EINVAL can be returned otherwise.

I'll add an isascii() loop in the next round.

Cheers, David
Bart Van Assche Dec. 4, 2018, 3:48 p.m. UTC | #5 
On Tue, 2018-12-04 at 16:26 +0300, Roman Bolshakov wrote:
> wrt PATCH 5 in the series. Should we allow to set vendor_id for for
> pscsi?

I think we should allow that.

Bart.
diff mbox series

Patch

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index 34872f24e8bf..67303c3f9cb4 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -1217,6 +1217,52 @@  static struct t10_wwn *to_t10_wwn(struct config_item *item)
 }
 
 /*
+ * STANDARD and VPD page 0x80 T10 Vendor Identification
+ */
+static ssize_t target_wwn_vendor_id_show(struct config_item *item,
+		char *page)
+{
+	return sprintf(page, "%s\n", &to_t10_wwn(item)->vendor[0]);
+}
+
+static ssize_t target_wwn_vendor_id_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct t10_wwn *t10_wwn = to_t10_wwn(item);
+	struct se_device *dev = t10_wwn->t10_dev;
+	unsigned char buf[INQUIRY_VENDOR_LEN + 1];
+
+	if (strlen(page) > INQUIRY_VENDOR_LEN) {
+		pr_err("Emulated T10 Vendor Identification exceeds"
+			" INQUIRY_VENDOR_LEN: " __stringify(INQUIRY_VENDOR_LEN)
+			"\n");
+		return -EOVERFLOW;
+	}
+	strlcpy(buf, page, sizeof(buf));
+	/*
+	 * Check to see if any active exports exist.  If they do exist, fail
+	 * here as changing this information on the fly (underneath the
+	 * initiator side OS dependent multipath code) could cause negative
+	 * effects.
+	 */
+	if (dev->export_count) {
+		pr_err("Unable to set T10 Vendor Identification while"
+			" active %d exports exist\n", dev->export_count);
+		return -EINVAL;
+	}
+
+	/* Assume ASCII encoding. Strip any newline added from userspace. */
+	BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
+	strlcpy(dev->t10_wwn.vendor, strstrip(buf),
+		sizeof(dev->t10_wwn.vendor));
+
+	pr_debug("Target_Core_ConfigFS: Set emulated T10 Vendor Identification:"
+		 " %s\n", dev->t10_wwn.vendor);
+
+	return count;
+}
+
+/*
  * VPD page 0x80 Unit serial
  */
 static ssize_t target_wwn_vpd_unit_serial_show(struct config_item *item,
@@ -1362,6 +1408,7 @@  DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_target_port, 0x10);
 /* VPD page 0x83 Association: SCSI Target Device */
 DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_scsi_target_device, 0x20);
 
+CONFIGFS_ATTR(target_wwn_, vendor_id);
 CONFIGFS_ATTR(target_wwn_, vpd_unit_serial);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_protocol_identifier);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_logical_unit);
@@ -1369,6 +1416,7 @@  CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_target_port);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_scsi_target_device);
 
 static struct configfs_attribute *target_core_dev_wwn_attrs[] = {
+	&target_wwn_attr_vendor_id,
 	&target_wwn_attr_vpd_unit_serial,
 	&target_wwn_attr_vpd_protocol_identifier,
 	&target_wwn_attr_vpd_assoc_logical_unit,