[libvirt] [PATCH] Fix usage of virFileMakePath, it doesn't set errno but returns it

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110705/3f84811d/attachment-0001.sig>