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