On 07/05/2011 08:00 AM, Matthias Bolte wrote:
> Also be explicity about the != 0 check in the few places that weren't.
s/explicity/explicit/
> ---
> src/conf/domain_conf.c | 6 +++---
> src/conf/network_conf.c | 2 +-
> src/conf/nwfilter_conf.c | 4 ++--
> src/conf/storage_conf.c | 2 +-
> src/libxl/libxl_driver.c | 20 ++++++++++----------
> src/lxc/lxc_container.c | 16 ++++++++--------
> src/lxc/lxc_controller.c | 6 +++---
> src/lxc/lxc_driver.c | 2 +-
> src/network/bridge_driver.c | 6 +++---
> src/qemu/qemu_driver.c | 28 ++++++++++++++--------------
> src/qemu/qemu_process.c | 6 +++---
> src/storage/storage_driver.c | 2 +-
> src/uml/uml_driver.c | 13 +++++++------
> src/util/dnsmasq.c | 2 +-
> src/util/util.c | 2 +-
> 15 files changed, 59 insertions(+), 58 deletions(-)
As long as we are already auditing all callers,...
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 109a947..2467fcf 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -9980,14 +9980,14 @@ int virDomainSaveXML(const char *configDir,
> const char *xml)
> {
> char *configFile = NULL;
> - int fd = -1, ret = -1;
> + int fd = -1, ret = -1, err;
> size_t towrite;
>
> if ((configFile = virDomainConfigFile(configDir, def->name)) == NULL)
> goto cleanup;
>
> - if (virFileMakePath(configDir)) {
> - virReportSystemError(errno,
> + if ((err = virFileMakePath(configDir)) != 0) {
> + virReportSystemError(err,
I'm half inclined to NACK this patch, and instead request that we fix
virFileMakePath to be more like the bulk of our code: return 0 on
success, -1 on error, and guarantee that errno is valid on error.
Then we can simply write:
if (virFileMakePath(configDir) < 0) {
virReportSystemError(errno, ...
without needing an extra 'err' variable.
> +++ b/src/util/util.c
> @@ -1182,7 +1182,7 @@ int virFileWritePid(const char *dir,
> goto cleanup;
> }
>
> - if ((rc = virFileMakePath(dir)))
> + if ((rc = virFileMakePath(dir)) != 0)
The biggest problem with this patch is that virFileMakePath still has no
documentation. I guess I could be okay with the approach in this patch
if it were to also include a comment just before virFileMakePath
documenting that on error the return is a positive(!) errno value, if
that means that fewer callers have to change, although I still feel that
going with our normal conventions of a negative return on error would be
a better cleanup. At any rate, until virFileMakePath is documented, we
will continue to have problems with clients using it incorrectly.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
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