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).
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
