10:10 a.m.
Daniel P. Berrange wrote:
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.
FYI, I've pushed this:
From fb54230b60be19ccdad39fd723d7d75f3a920808 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Mon, 25 Jan 2010 16:54:06 +0100
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.
---
src/util/hostusb.c | 26 +++++++++++++++-----------
1 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/src/util/hostusb.c b/src/util/hostusb.c
index f635ce5..71f6435 100644
--- a/src/util/hostusb.c
+++ b/src/util/hostusb.c
@@ -70,20 +70,20 @@ static int usbSysReadFile(virConnectPtr conn,
tmp = virAsprintf(&filename, USB_SYSFS "/devices/%s/%s", d_name,
f_name);
if (tmp < 0) {
virReportOOMError(conn);
- goto error;
+ goto cleanup;
}
if (virFileReadAll(filename, 1024, &buf) < 0)
- goto error;
+ goto cleanup;
if (virStrToLong_ui(buf, &ignore, base, value) < 0) {
usbReportError(conn, VIR_ERR_INTERNAL_ERROR,
_("Could not parse usb file %s"), filename);
- goto error;
+ goto cleanup;
}
ret = 0;
-error:
+cleanup:
VIR_FREE(filename);
VIR_FREE(buf);
return ret;
@@ -103,7 +103,7 @@ static int usbFindBusByVendor(virConnectPtr conn,
virReportSystemError(conn, errno,
_("Could not open directory %s"),
USB_SYSFS "/devices");
- goto error;
+ goto cleanup;
}
while ((de = readdir(dir))) {
@@ -113,10 +113,10 @@ static int usbFindBusByVendor(virConnectPtr conn,
if (usbSysReadFile(conn, "idVendor", de->d_name,
16, &found_vend) < 0)
- goto error;
+ goto cleanup;
if (usbSysReadFile(conn, "idProduct", de->d_name,
16, &found_prod) < 0)
- goto error;
+ goto cleanup;
if (found_prod == product && found_vend == vendor) {
/* Lookup bus.addr info */
@@ -130,12 +130,12 @@ static int usbFindBusByVendor(virConnectPtr conn,
usbReportError(conn, VIR_ERR_INTERNAL_ERROR,
_("Failed to parse dir name '%s'"),
de->d_name);
- goto error;
+ goto cleanup;
}
if (usbSysReadFile(conn, "devnum", de->d_name,
10, &found_addr) < 0)
- goto error;
+ goto cleanup;
*bus = found_bus;
*devno = found_addr;
@@ -150,8 +150,12 @@ static int usbFindBusByVendor(virConnectPtr conn,
else
ret = 0;
-error:
- closedir (dir);
+cleanup:
+ if (dir) {
+ int saved_errno = errno;
+ closedir (dir);
+ errno = saved_errno;
+ }
return ret;
}
--
1.7.0.rc0.140.gfbe7