Thread (13 messages) 13 messages, 5 authors, 2021-10-12

Re: [PATCH v5 1/7] target: core: add common tpg/enable attribute

From: Bodo Stroesser <bostroesser@gmail.com>
Date: 2021-09-15 18:00:19
Also in: target-devel

On 10.09.21 10:41, Dmitry Bogdanov wrote:
quoted hunk ↗ jump to hunk
Many fabric modules provide their own implementation of enable
attribute in tpg.
The change provides a way to remove code duplication in the fabric
modules and automatically add "enable" attribute if a fabric module has
an implementation of fabric_enable_tpg() ops.

Reviewed-by: Roman Bolshakov <redacted>
Signed-off-by: Dmitry Bogdanov <redacted>
---
  drivers/target/target_core_configfs.c        |  1 +
  drivers/target/target_core_fabric_configfs.c | 78 +++++++++++++++++++-
  include/target/target_core_base.h            |  1 +
  include/target/target_core_fabric.h          |  1 +
  4 files changed, 79 insertions(+), 2 deletions(-)
diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index 102ec644bc8a..3b9e50c1ccef 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -490,6 +490,7 @@ void target_unregister_template(const struct target_core_fabric_ops *fo)
  			 * fabric driver unload of TFO->module to proceed.
  			 */
  			rcu_barrier();
+			kfree(t->tf_tpg_base_cit.ct_attrs);
  			kfree(t);
  			return;
  		}
diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index fc7edc04ee09..0b65de9f2df1 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -815,8 +815,76 @@ static struct configfs_item_operations target_fabric_tpg_base_item_ops = {
  	.release		= target_fabric_tpg_release,
  };
  
-TF_CIT_SETUP_DRV(tpg_base, &target_fabric_tpg_base_item_ops, NULL);
+static ssize_t target_fabric_tpg_base_enable_show(struct config_item *item,
+						  char *page)
+{
+	return sysfs_emit(page, "%d\n", to_tpg(item)->enabled);
+}
+
+static ssize_t target_fabric_tpg_base_enable_store(struct config_item *item,
+						   const char *page,
+						   size_t count)
+{
+	struct se_portal_group *se_tpg = to_tpg(item);
+	int ret;
+	bool op;
+
+	ret = strtobool(page, &op);
+	if (ret)
+		return ret;
+
+	if (se_tpg->enabled == op)
+		return count;
Sorry for jumping in lately.

Just one nit:
In case someone tries to enable or disable the same tpg a second time,
with the change we always do nothing and return count (--> OK).

I just checked iscsi and qla2xxx. AFAICS iscsi before the patch rejected
the second enable or disable with -EINVAL, while qla2xxx accepts the
second disable and rejects the second enable with -EEXIST.

Of course it sounds good to unify the behavior of existing enable
attributes. OTOH: even if enabling/disabling the same tpg twice can be
seen as suspicious behavior, are we sure to not confuse existing user 
space tools by changing the result?

Bodo

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help