Hi Tregaron,
On Tue, 2013-01-29 at 14:00 -0700, Tregaron Bayly wrote:
> On Tue, 2013-01-29 at 11:30 -0800, Nicholas A. Bellinger wrote:
>
> > This bottom section is useless here, as config_item_name() will never
> > return NULL for a normal se_device.
>
> I only had it in as a defensive measure - kernel panics are ugly so I
> thought a sanity check wouldn't be a bad idea. If we are guaranteed to
> never return NULL and you hate the code then I can remove the check.
>
Correct, config_item_name() does not need NULL checking (see existing
fs/configfs code usage), as struct config_item->ci_name will always be
set by configfs one way or another once a struct se_device->dev_group
has been successfully registered.
> > This is still missing the ability to pass flag=0 to revert back to the
> > old model name.
> >
> > Please make sure that any option that can be enabled via configfs, can
> > also be disabled via configfs.
>
> Yes, that is bad. I'll fix.
>
> > > @@ -1468,8 +1510,12 @@ int target_configure_device(struct se_device *dev)
> > > */
> > > if (dev->transport->transport_type != TRANSPORT_PLUGIN_PHBA_PDEV) {
> > > strncpy(&dev->t10_wwn.vendor[0], "LIO-ORG", 8);
> > > - strncpy(&dev->t10_wwn.model[0],
> > > - dev->transport->inquiry_prod, 16);
> > > + if (dev->dev_attrib.emulate_model_alias) {
> > > + dev_set_t10_wwn_model_alias(dev);
> > > + } else {
> > > + strncpy(&dev->t10_wwn.model[0],
> > > + dev->transport->inquiry_prod, 16);
> > > + }
> >
> > Please drop this code for the non default usage case.
> >
> > If someone want's to make this the default action, then they should
> > modify user-space to set emulate_model_alias=1.
>
> I'm not sure I understand what change you're looking for here. You
> always want the device to be built with dev->transport->inquiry_prod and
> then only changed by dev_set_t10_wwn_model_alias() at the time the
> administrator modifies the user-space option?
Correct. Doing this is user-space code is what I'd like to see.
> What about people like me
> who set DA_EMULATE_MODEL_ALIAS to 1 and compile?
For your case you'll want to modify rtslib and/or targetcli to do this
by default, so you don't need to make changes to an upstream kernel.
> Doesn't it make more
> sense to check the option at the time we configure the device so that it
> does the right thing according the value of the config option at that
> moment (however it got that way)?
Since the default for emulate_model_alias will need to remain zero for
the foreseeable future, this ends up being dead code for an upstream
patch.
I generally avoid adding code like this for non-default cases,
especially when it's easier to simply setting this bit (from userspace)
with a couple of lines of python code in user-space.
--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
