[libvirt] [PATCH] usbFindBusByVendor: don't leak a DIR buffer and FD

[Daniel P. Berrange]

On Tue, Jan 26, 2010 at 03:59:08PM +0100, Jim Meyering wrote:
> Eric Blake wrote:
> 
> > According to Jim Meyering on 1/25/2010 8:55 AM:
> >> Subject: [PATCH] hostusb: closedir only if non-NULL; rename labels: s/error/cleanup/
> >>
> >> * src/util/hostusb.c (usbSysReadFile): Rename labels s/error/cleanup/
> >> (usbFindBusByVendor): Likewise.  And closedir only if non-NULL.
> >> @@ -150,8 +150,9 @@ static int usbFindBusByVendor(virConnectPtr conn,
> >>      else
> >>          ret = 0;
> >>
> >> -error:
> >> -    closedir (dir);
> >> +cleanup:
> >> +    if (dir)
> >> +        closedir (dir);
> >
> > Should errno be saved and restored around this point, to avoid it being
> > arbitrarily changed by closedir?
> 
> Thanks for looking.
> Technically you're right, and I'll merge this:
> 
> diff --git a/src/util/hostusb.c b/src/util/hostusb.c
> index 72d0833..71f6435 100644
> --- a/src/util/hostusb.c
> +++ b/src/util/hostusb.c
> @@ -151,8 +151,11 @@ static int usbFindBusByVendor(virConnectPtr conn,
>          ret = 0;
> 
>  cleanup:
> -    if (dir)
> +    if (dir) {
> +        int saved_errno = errno;
>          closedir (dir);
> +        errno = saved_errno;
> +    }
>      return ret;
>  }
> 
> However, no caller that I have seen uses errno (I looked at all
> direct callers and stopped after getting to 4th or 5th-level indirect
> callers in one part of the call tree), so currently it makes no difference.

Yep, generally speaking if a caller needs to be given back the actual
errno value, then the function should be returing  '-errno' instead 
of the fixed -1.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|