Re: [libvirt] [PATCHv2 4/4] Remove erroneous setting of return value to errno.
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).