Re: [PATCH v3 3/6] [SCSI] Add support for scsi_target events

[Ewan Milne <emilne@redhat.com>]

On Wed, 2013-06-19 at 19:54 +0200, Bart Van Assche wrote:
> On 06/19/13 19:42, Ewan D. Milne wrote:
> > +static void starget_evt_emit(struct scsi_target *starget,
> > +			     struct starget_event *evt)
> > +{
> > +	int idx = 0;
> > +	char *envp[3];
> > +
> > +	switch (evt->evt_type) {
> > +	case STARGET_EVT_LUN_CHANGE_REPORTED:
> > +		envp[idx++] = "STARGET_LUN_CHANGE_REPORTED=1";
> > +		break;
> > +	default:
> > +		/* do nothing */
> > +		break;
> > +	}
> > +
> > +	envp[idx++] = NULL;
> > +
> > +	kobject_uevent_env(&starget->dev.kobj, KOBJ_CHANGE, envp);
> > +}
> 
> Sorry but it's not clear to me why the envp[] array has size three while 
> at most two entries are used ? And shouldn't that array be declared as 
> const char*[] instead of char *[] since string constants have type const 
> char[] ?

Hmm.  I copied the code from the (renamed) scsi_evt_emit() which had
char *envp[3], but you are right, only 2 entries are needed.  As far
as the const attribute goes, the signature of kobject_uevent_env() in
include/linux/kobject.h doesn't have the const attribute on the envp
parameter, and other callers don't use const.

> 
> > +		list_for_each_safe(this, tmp, &event_list) {
> > +			evt = list_entry(this, struct starget_event, node);
> 
> Any reason why list_for_each_entry_safe() has not been used here ?

No particular reason, again the code was copied from the (renamed)
sdev_evt_thread(). 

> 
> > +void starget_evt_send(struct scsi_target *starget, struct starget_event *evt)
> > +{
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&starget->list_lock, flags);
> > +	list_add_tail(&evt->node, &starget->event_list);
> > +	schedule_work(&starget->event_work);
> > +	spin_unlock_irqrestore(&starget->list_lock, flags);
> > +}
> 
> Is it necessary here to invoke schedule_work() under protection of 
> list_lock, or would it be safe to invoke schedule_work() after the 
> spin_unlock_irqrestore() ?

I think the call to schedule_work() can be moved outside, the lock, I'll
have to verify that.

> 
> > +#ifdef CONFIG_SCSI_ENHANCED_UA
> > +	struct list_head *this, *tmp;
> > +
> > +	cancel_work_sync(&starget->event_work);
> > +
> > +	list_for_each_safe(this, tmp, &starget->event_list) {
> > +		struct starget_event *evt;
> >
> > +		evt = list_entry(this, struct starget_event, node);
> > +		list_del(&evt->node);
> > +		kfree(evt);
> > +	}
> > +#endif
> 
> Same question here: any reason why list_for_each_entry_safe() has not 
> been used ?

Same answer:  the code came from scsi_device_dev_release_usercontext().

I see that James would prefer that the events be unified rather than
specific to scsi_device or scsi_target, so I'll remove the duplication.

Thanks for the comments.

-Ewan

> 
> Thanks,
> 
> Bart.