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

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

 



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@xxxxxxxxxx>
> ---
>  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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]