[libvirt] [PATCHv2 4/4] Remove erroneous setting of return value to errno.
laine at laine.org
Wed Jul 21 21:29:57 UTC 2010
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
>>> 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
> -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
> "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
> "return -1 on error") as well.
> In order to do that, I would have to 1) change all other functions
> called interchangeably with this function (ie, anything assigned to
> in storage_backend_fs.c, assigned to .buildVolFrom, or a couple other
> and 2) assure that all places any of these are called are okay with a
> 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
> by some system failure).
> Instead of pushing this change right away, I'll go through all those
> 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).
More information about the libvir-list