[libvirt] [PATCHv2 4/4] Remove erroneous setting of return value to errno.

[Laine Stump]

  On 07/21/2010 02:28 PM, Laine Stump wrote:
>  On 07/21/2010 11:37 AM, Eric Blake wrote:
>> On 07/21/2010 09:21 AM, Daniel Veillard wrote:
>>>    ACK, but it seems to me ret being initialized to -1, maybe it's 
>>> better
>>> to be consistent and on all error do a ret = -errno; before
>>> virReportSystemError and the goto cleanup.
>> If you have the convention of returning -errno, then you must not return
>> -1 unless you meant to report EPERM.  I agree with Laine's approach of
>> initializing to 0 in this case, since you don't want to leak an
>> unintended -1 if that was not the real error.
>>
>

 > In the case of this function, the patch isn't intended to make the 
function return
 > -errno (like the others in this series), it was just to make it 
consistent; this was
 > a bug that I coincidentally noticed while changing the other 
functions from
 > "return positive errno on failure" to "return -errno on failure". I 
think DV was
 > wondering out loud if we should change the convention of this 
function (from
 > "return -1 on error") as well.

 > In order to do that, I would have to 1) change all other functions 
that are
 > called interchangeably with this function (ie, anything assigned to 
create_func
 > in storage_backend_fs.c, assigned to .buildVolFrom, or a couple other 
places),
 > and 2) assure that all places any of these are called are okay with a 
return
 > that is merely < 0, rather than explicitly -1. (and we would also 
need to make
 > sure that there was a reasonable errno to return in every error case; for
 > example in some functions the error is caused by a configuration 
problem, not
 > by some system failure).
 >
 > Instead of pushing this change right away, I'll go through all those 
functions
 > to see if such a change is possible.


I looked at this more, and saw that this function is just one of "many" 
(probably a couple dozen) functions that are used by the storage driver 
and pointed to by members of virStorageBackend tables. All of these 
functions consistently return 0 on success and -1 on error. If I were 
going to change this one function (virStorageBackendCreateBlockFrom), I 
should then change any function that could be set as a .buildFrom member 
of virStorageBackend, and if I did that, I should change all functions 
that could be any of the members of virStorageBackend. The problem is 
that in some cases, there is no appropriate errno to return, and none of 
the callers expect that anyway.


So in the end, I've decided to leave this function returning -1 on 
failure, and 0 on success (and am pushing the patch as-is, since it 
eliminates something that violates that convention).