[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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).

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]