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

Re: [libvirt] [PATCH 1/3] Add missing error reporting

On 04/18/2013 04:30 AM, Martin Kletzander wrote:
> On two places, there were errors not being reported.  One strdup
> without virReportOOMError() and call for virFileMakePathHelper() which
> doesn't report any errors, just sets errno (or leaves it set by
> underlying functions).
> Signed-off-by: Martin Kletzander <mkletzan redhat com>
> ---
>  src/util/virutil.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Needs work.  Right now, this function intentionally does not report
errors; all direct callers (src/util/virlockspace.c and tools/virsh.c)
are aware of this convention.

On the other hand, I looked through indirect callers of
virFileMakePath(), and found at least the following problems:

daemon/libvirtd.c: daemonPidFilePath() doesn't report an error
src/conf/*_conf.c: 5 callers report error
libxl_driver.c: reports errors with VIR_ERROR instead of
virReportSystemError, so it gets logged but not reported to caller
src/locking/lock_daemon.c: same bug as daemon/libvirt.c

I stopped looking here - lots of other callers, and probably a similar
mix of functions that report the error themselves, vs. functions that
fail to do error reporting.

We need to decide which way to go, then audit ALL users to obey that
convention.  Note that there are cases (such as tools/virsh.c) that
intentionally want to suppress errors of EEXIST, except that
virFileMakePathHelper already guarantees that EEXIST won't be the cause
of failure, so maybe cleaning up ALL callers to expect virutil.c to do
error reporting is the way to go.

Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

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