Message ID | 1464805178-8989-2-git-send-email-cjp256@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Chris Patterson writes ("[[PATCH v2 2/2] libxl: replace deprecated readdir_r() with readdir()"): > - for (;;) { > + while ((de = readdir(dir)) != NULL) { ... > - int r = readdir_r(dir, de_buf, &de); > - if (r) { > - LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV); > - break; Sadly this is not right because it mishandles errors when reading the directory, treating them all as EOF. See the error handling info in the specification for readdir: http://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html Ian.
On Thu, Jun 2, 2016 at 6:11 AM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: > Chris Patterson writes ("[[PATCH v2 2/2] libxl: replace deprecated readdir_r() with readdir()"): >> - for (;;) { >> + while ((de = readdir(dir)) != NULL) { > ... >> - int r = readdir_r(dir, de_buf, &de); >> - if (r) { >> - LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV); >> - break; > > Sadly this is not right because it mishandles errors when reading the > directory, treating them all as EOF. See the error handling info > in the specification for readdir: > http://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html > You're right, it should check for the error afterwards. How about something along the lines of: int saved_errno = errno; errno = 0; while ((de = readdir(dir)) != NULL) { ... } if (errno) { LOGE(ERROR, "readdir failed: %s", strerror(errno)); rc = ERROR_FAIL; } errno = saved_errno; ...
Chris Patterson writes ("Re: [[PATCH v2 2/2] libxl: replace deprecated readdir_r() with readdir()"): > You're right, it should check for the error afterwards. > > How about something along the lines of: > > int saved_errno = errno; > errno = 0; > while ((de = readdir(dir)) != NULL) { > ... Wrong because you need to set errno=0 before each call to readdir. I really think you should abandon your efforts to keep the readdir call inside the while() condition :-). > if (errno) { > LOGE(ERROR, "readdir failed: %s", strerror(errno)); > rc = ERROR_FAIL; > } > errno = saved_errno; I haven't eyeballed the context in detail but I don't understand why you think it necessary to save and restore errno. All the many system and library calls made throughout this code may overwrite it anyway. Ian.
On Thu, Jun 2, 2016 at 12:13 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: > Chris Patterson writes ("Re: [[PATCH v2 2/2] libxl: replace deprecated readdir_r() with readdir()"): >> You're right, it should check for the error afterwards. >> >> How about something along the lines of: >> >> int saved_errno = errno; >> errno = 0; >> while ((de = readdir(dir)) != NULL) { >> ... > > Wrong because you need to set errno=0 before each call to readdir. > I really think you should abandon your efforts to keep the readdir > call inside the while() condition :-). > I agree. How does something like this look? - int r = readdir_r(dir, de_buf, &de); - - if (r) { + errno = 0; + de = readdir(dir); + + if (!de && errno) { And I'll apply the same construct for tools/libfsimage/common/fsimage_plugin.c.
diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c index 9f1e842..01819bd 100644 --- a/tools/libxl/libxl_pvusb.c +++ b/tools/libxl/libxl_pvusb.c @@ -508,19 +508,10 @@ int libxl_devid_to_device_usbctrl(libxl_ctx *ctx, return rc; } -static void *zalloc_dirent(libxl__gc *gc, const char *dirpath) -{ - size_t need = offsetof(struct dirent, d_name) + - pathconf(dirpath, _PC_NAME_MAX) + 1; - - return libxl__zalloc(gc, need); -} - static char *usbdev_busaddr_to_busid(libxl__gc *gc, int bus, int addr) { DIR *dir; char *busid = NULL; - struct dirent *de_buf; struct dirent *de; /* invalid hostbus or hostaddr */ @@ -533,22 +524,12 @@ static char *usbdev_busaddr_to_busid(libxl__gc *gc, int bus, int addr) return NULL; } - de_buf = zalloc_dirent(gc, SYSFS_USB_DEV); - - for (;;) { + while ((de = readdir(dir)) != NULL) { char *filename; void *buf; int busnum = -1; int devnum = -1; - int r = readdir_r(dir, de_buf, &de); - if (r) { - LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV); - break; - } - if (!de) - break; - if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, "..")) continue; @@ -1157,9 +1138,7 @@ static int usbdev_get_all_interfaces(libxl__gc *gc, const char *busid, { DIR *dir; char *buf; - struct dirent *de_buf; struct dirent *de; - int rc; *intfs = NULL; *num = 0; @@ -1172,19 +1151,7 @@ static int usbdev_get_all_interfaces(libxl__gc *gc, const char *busid, return ERROR_FAIL; } - de_buf = zalloc_dirent(gc, SYSFS_USB_DEV); - - for (;;) { - int r = readdir_r(dir, de_buf, &de); - - if (r) { - LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV); - rc = ERROR_FAIL; - goto out; - } - if (!de) - break; - + while ((de = readdir(dir)) != NULL) { if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, "..")) continue; @@ -1196,11 +1163,8 @@ static int usbdev_get_all_interfaces(libxl__gc *gc, const char *busid, } } - rc = 0; - -out: closedir(dir); - return rc; + return 0; } /* Encode usb interface so that it could be written to xenstore as a key. diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c index ceb8825..5730774 100644 --- a/tools/libxl/libxl_utils.c +++ b/tools/libxl/libxl_utils.c @@ -548,21 +548,9 @@ int libxl__remove_directory(libxl__gc *gc, const char *dirpath) goto out; } - size_t need = offsetof(struct dirent, d_name) + - pathconf(dirpath, _PC_NAME_MAX) + 1; - struct dirent *de_buf = libxl__zalloc(gc, need); struct dirent *de; - for (;;) { - int r = readdir_r(d, de_buf, &de); - if (r) { - LOGE(ERROR, "failed to readdir %s for removal", dirpath); - rc = ERROR_FAIL; - break; - } - if (!de) - break; - + while ((de = readdir(d)) != NULL) { if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, "..")) continue;