iscsi_add_session() warning

iscsi_add_session() warning

[Andrew Morton]

Looking at this:

drivers/scsi/scsi_transport_iscsi.c: In function 'iscsi_add_session':
drivers/scsi/scsi_transport_iscsi.c:703: warning: 'err' may be used uninitialized in this function

the compiler is wrong - `err' will always have been initialised to
_something_.

But to what?

: int iscsi_add_session(struct iscsi_cls_session *session, unsigned int target_id)
: {
: 	struct Scsi_Host *shost = iscsi_session_to_shost(session);
: 	struct iscsi_cls_host *ihost;
: 	unsigned long flags;
: 	unsigned int id = target_id;
: 	int err;
: 
: 	ihost = shost->shost_data;
: 	session->sid = atomic_add_return(1, &iscsi_session_nr);
: 
: 	if (id == ISCSI_MAX_TARGET) {
: 		for (id = 0; id < ISCSI_MAX_TARGET; id++) {
: 			err = device_for_each_child(&shost->shost_gendev, &id,
: 						    iscsi_get_next_target_id);
: 			if (!err)
: 				break;
: 		}
: 
: 		if (id == ISCSI_MAX_TARGET) {
: 			iscsi_cls_session_printk(KERN_ERR, session,
: 						 "Too many iscsi targets. Max "
: 						 "number of targets is %d.\n",
: 						 ISCSI_MAX_TARGET - 1);

At this point, err is probably zero.  Or it might be the return value
from the most recent call to
device_for_each_child(iscsi_get_next_target_id).

Should we not assign some error code to `err' here?

: 			goto release_host;
: 		}
: 	}
: 	session->target_id = id;
: 
: 	dev_set_name(&session->dev, "session%u", session->sid);
: 	err = device_add(&session->dev);
: 	if (err) {
: 		iscsi_cls_session_printk(KERN_ERR, session,
: 					 "could not register session's dev\n");
: 		goto release_host;
: 	}
: 	transport_register_device(&session->dev);
: 
: 	spin_lock_irqsave(&sesslock, flags);
: 	list_add(&session->sess_list, &sesslist);
: 	spin_unlock_irqrestore(&sesslock, flags);
: 
: 	iscsi_session_event(session, ISCSI_KEVENT_CREATE_SESSION);
: 	return 0;
: 
: release_host:
: 	scsi_host_put(shost);
: 	return err;
: }

Re: iscsi_add_session() warning

[Kyle McMartin]

On Tue, Feb 03, 2009 at 04:09:00PM -0800, Andrew Morton wrote:
> : 	if (id == ISCSI_MAX_TARGET) {
> : 		for (id = 0; id < ISCSI_MAX_TARGET; id++) {
> : 			err = device_for_each_child(&shost->shost_gendev, &id,

Possibly GCC just can't figure out that id was reinitialized, so it
thinks that the for loop won't be executed?

Just a guess...
	Kyle

Re: iscsi_add_session() warning

[Andrew Morton]

On Tue, 3 Feb 2009 19:12:38 -0500
Kyle McMartin <kyle@infradead.org> wrote:

> On Tue, Feb 03, 2009 at 04:09:00PM -0800, Andrew Morton wrote:
> > : 	if (id == ISCSI_MAX_TARGET) {
> > : 		for (id = 0; id < ISCSI_MAX_TARGET; id++) {
> > : 			err = device_for_each_child(&shost->shost_gendev, &id,
> 
> Possibly GCC just can't figure out that id was reinitialized, so it
> thinks that the for loop won't be executed?
> 

Could be.  But that isn't the point...

The point, dear Kyle, is that I misread the code :(

In fact on that codepath the function _will_ return the most recent
return value from device_for_each_child(iscsi_get_next_target_id), and
that value will be non-zero (-EEXIST).  So it looks non-buggy, albeit
rather obscure.

But did we intend to return -EEXIST in the "Too many iscsi targets" case?

Re: iscsi_add_session() warning

[Mike Christie]

Andrew Morton wrote:
> On Tue, 3 Feb 2009 19:12:38 -0500
> Kyle McMartin <kyle@infradead.org> wrote:
> 
>> On Tue, Feb 03, 2009 at 04:09:00PM -0800, Andrew Morton wrote:
>>> : 	if (id == ISCSI_MAX_TARGET) {
>>> : 		for (id = 0; id < ISCSI_MAX_TARGET; id++) {
>>> : 			err = device_for_each_child(&shost->shost_gendev, &id,
>> Possibly GCC just can't figure out that id was reinitialized, so it
>> thinks that the for loop won't be executed?
>>
> 
> Could be.  But that isn't the point...
> 
> The point, dear Kyle, is that I misread the code :(
> 
> In fact on that codepath the function _will_ return the most recent
> return value from device_for_each_child(iscsi_get_next_target_id), and
> that value will be non-zero (-EEXIST).  So it looks non-buggy, albeit
> rather obscure.
> 
> But did we intend to return -EEXIST in the "Too many iscsi targets" case?

I did not. The callers just check for non zero so it will work. It would 
be best to change it to something meaningful.

When there are no ids for some object left, what is the stadard Exxxx 
value to return? Would ENOSPC make sense? Here ENOSPC would mean no 
space in id space left instead of space on a device?

Re: iscsi_add_session() warning

[Andrew Morton]

On Wed, 04 Feb 2009 01:40:11 -0600 Mike Christie <michaelc@cs.wisc.edu> wrote:

> When there are no ids for some object left, what is the stadard Exxxx 
> value to return? Would ENOSPC make sense? Here ENOSPC would mean no 
> space in id space left instead of space on a device?

Drivers often do that sort of thing.  I don't like it personally -
ENOSPC means "No space left on device".  IOW, "your disk is full".

Co-opting that code just because something kinda-sorta-similar happened
seems abusive to me.  Plus the first thing poor old user is going to do
is run `df' and ask wtf?

So what's left?  EINVAL is a sort of generic something-went-wrong error
code (in Linux, at least).  But perhaps EBUSY ("Device or resource
busy") is suitable here.

Dunno, hard.

Re: iscsi_add_session() warning

[Grant Grundler]

On Tue, Feb 3, 2009 at 4:12 PM, Kyle McMartin <kyle@infradead.org> wrote:
> On Tue, Feb 03, 2009 at 04:09:00PM -0800, Andrew Morton wrote:
>> :     if (id == ISCSI_MAX_TARGET) {
>> :             for (id = 0; id < ISCSI_MAX_TARGET; id++) {
>> :                     err = device_for_each_child(&shost->shost_gendev, &id,
>
> Possibly GCC just can't figure out that id was reinitialized, so it
> thinks that the for loop won't be executed?

Would restructuring the code slightly be ok?

  if (target_id ==  ISCSI_MAX_TARGET) {
     unsigned int id = 0;
...
     session->target_id = id;
  } else {
     session->target_id = target_id;
  }
...

ie. reduce the scope of "id" . Not quite a concise as existing code
but just as readable.

hth,
grant

> Just a guess...
>        Kyle
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>