On Mon, 2011-11-28 at 13:25 +0100, Sebastian Andrzej Siewior wrote: > * Nicholas A. Bellinger | 2011-11-28 02:50:45 [-0800]: > > >Hi again Sebastian, > > > >I like this cleanup using se_device->se_dev_udev_path instead of > >internal struct member usage for iblock, but this will currently break > >userspace during iblock device setup when returning -EINVAL for existing > >iblock_set_configfs_dev_params() usage. > > > >So I'd like to avoid this for the moment as we still expect all backend > >devices to use some form of target/core/$HBA/$DEV/control input, and > >while making a special case for IBLOCK in userspace may be a future > >option, it's not worth the pain to userspace breakage w/ this patch atm. > > Okay, I wasn't aware that I'm breaking something. > > >The other option here would be to return non exception status when using > >target/core/$HBA/$DEV/control for backend drivers (like IBLOCK) that > >would no longer provide se_subsystem_api->set_configfs_dev_params() from > >target_core_store_dev_control() usage. This would still expect to fail > >during target_core_store_dev_enable() -> check_configfs_dev_params() if > >udev_path has not been set via se_device->se_dev_udev_path, so returning > >non exception status here from legacy control attribute input should be > >enough to make existing userspace happy with iblock backend. > > Yeah but not returning an error + doing nothing and exploding later > duing enable is kinda wrong I think. What about this: > <nod> > >From 48b67d6cc07c2397736c06dba9b28bd959022e96 Mon Sep 17 00:00:00 2001 > From: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> > Date: Thu, 24 Nov 2011 14:01:26 +0100 > Subject: [PATCH] target/iblock: use se_dev_udev_path instead of ibd_udev_path > > This is probably a leftover from a rework. According to the wiki > | echo iblock_major=254,iblock_minor=2 > $TARGET/iblock_0/lvm_test0/control > > should be issued. However the code matches for udev_path and force. > Since we have already udev_path handling in target_core_configfs lets > use it. I remove the major/minor thingy since there is no evidence that > this is used. > This also provides compatibility for old users of control interface. > Once they are gone and no control user comes along, then this field can > be removed. > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> > --- > drivers/target/target_core_configfs.c | 3 +- > drivers/target/target_core_iblock.c | 45 ++++++++++++-------------------- > drivers/target/target_core_iblock.h | 3 -- > drivers/target/target_core_internal.h | 3 ++ > 4 files changed, 22 insertions(+), 32 deletions(-) > > diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c > index 3f75f1b..d7cce7b 100644 > --- a/drivers/target/target_core_configfs.c > +++ b/drivers/target/target_core_configfs.c > @@ -1740,7 +1740,7 @@ static ssize_t target_core_show_dev_udev_path(void *p, char *page) > return snprintf(page, PAGE_SIZE, "%s\n", se_dev->se_dev_udev_path); > } > > -static ssize_t target_core_store_dev_udev_path( > +ssize_t target_core_store_dev_udev_path( > void *p, > const char *page, > size_t count) > @@ -1770,6 +1770,7 @@ static ssize_t target_core_store_dev_udev_path( > > return read_bytes; > } > +EXPORT_SYMBOL_GPL(target_core_store_dev_udev_path); > I'd much rather just use your original patch instead of this one, and we really don't want backend drivers like iblock to use internal attribute store calls like this.. So that said, I'll consider your original patch as a >= v3.3 API change to /sys/kernel/config/target/core/, and get any left-over userspace breakage addressed with proper udev_path attribute usage. --nab -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
