6:53 a.m.
Introduce a wrapper for readdir. This helps us make sure that we always
set errno before calling readdir and it will make sure errors are
properly logged.
Signed-off-by: Natanael Copa <ncopa(a)alpinelinux.org>
---
src/util/virfile.c | 14 ++++++++++++++
src/util/virfile.h | 3 +++
2 files changed, 17 insertions(+)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 3eb2703..b54b9fd 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -2295,6 +2295,20 @@ virDirCreate(const char *path ATTRIBUTE_UNUSED,
}
#endif /* WIN32 */
+/* return 0 = success, 1 = end-of-dir and -1 = error */
+int virDirRead(DIR *dirp, struct dirent **ent, const char *dirname)
+{
+ errno = 0;
+ *ent = readdir(dirp);
+ if (!*ent && errno) {
+ if (dirname)
+ virReportSystemError(errno, _("Unable to read directory
'%s'"),
+ dirname);
+ return -1;
+ }
+ return !*ent;
+}
+
static int
virFileMakePathHelper(char *path, mode_t mode)
{
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 46ef781..622a81b 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -27,6 +27,7 @@
# define __VIR_FILE_H_
# include <stdio.h>
+# include <dirent.h>
# include "internal.h"
# include "virstoragefile.h"
@@ -211,6 +212,8 @@ enum {
};
int virDirCreate(const char *path, mode_t mode, uid_t uid, gid_t gid,
unsigned int flags) ATTRIBUTE_RETURN_CHECK;
+int virDirRead(DIR *dirp, struct dirent **ent, const char *dirname);
+
int virFileMakePath(const char *path) ATTRIBUTE_RETURN_CHECK;
int virFileMakePathWithMode(const char *path,
mode_t mode) ATTRIBUTE_RETURN_CHECK;
--
1.9.2
6:53 a.m.
New subject: [libvirt] [PATCH 2/2] nodeinfo: use virDirRead API
This makes sure that errno is reset before readdir is called, even if
the loop does a 'continue'.
This fixes issue with musl libc which sets errno on sscanf. The
following 'continue' makes the errno be set before calling readdir.
Signed-off-by: Natanael Copa <ncopa(a)alpinelinux.org>
---
src/nodeinfo.c | 29 ++++++++---------------------
1 file changed, 8 insertions(+), 21 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 53ba716..f239d4a 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -440,6 +440,7 @@ virNodeParseNode(const char *node,
int siblings;
unsigned int cpu;
int online;
+ int direrr;
*threads = 0;
*cores = 0;
@@ -452,8 +453,7 @@ virNodeParseNode(const char *node,
/* enumerate sockets in the node */
CPU_ZERO(&sock_map);
- errno = 0;
- while ((cpudirent = readdir(cpudir))) {
+ while ((direrr = virDirRead(cpudir, &cpudirent, node)) == 0) {
if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
continue;
@@ -470,14 +470,10 @@ virNodeParseNode(const char *node,
if (sock > sock_max)
sock_max = sock;
-
- errno = 0;
}
- if (errno) {
- virReportSystemError(errno, _("problem reading %s"), node);
+ if (direrr < 0)
goto cleanup;
- }
sock_max++;
@@ -490,8 +486,7 @@ virNodeParseNode(const char *node,
/* iterate over all CPU's in the node */
rewinddir(cpudir);
- errno = 0;
- while ((cpudirent = readdir(cpudir))) {
+ while ((direrr = virDirRead(cpudir, &cpudirent, node)) == 0) {
if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
continue;
@@ -530,14 +525,10 @@ virNodeParseNode(const char *node,
if (siblings > *threads)
*threads = siblings;
-
- errno = 0;
}
- if (errno) {
- virReportSystemError(errno, _("problem reading %s"), node);
+ if (direrr < 0)
goto cleanup;
- }
/* finalize the returned data */
*sockets = CPU_COUNT(&sock_map);
@@ -576,6 +567,7 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
int ret = -1;
char *sysfs_nodedir = NULL;
char *sysfs_cpudir = NULL;
+ int direrr;
/* Start with parsing CPU clock speed from /proc/cpuinfo */
while (fgets(line, sizeof(line), cpuinfo) != NULL) {
@@ -672,8 +664,7 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
goto fallback;
}
- errno = 0;
- while ((nodedirent = readdir(nodedir))) {
+ while ((direrr = virDirRead(nodedir, &nodedirent, sysfs_nodedir)) == 0) {
if (sscanf(nodedirent->d_name, "node%u", &node) != 1)
continue;
@@ -699,14 +690,10 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
if (threads > nodeinfo->threads)
nodeinfo->threads = threads;
-
- errno = 0;
}
- if (errno) {
- virReportSystemError(errno, _("problem reading %s"), sysfs_nodedir);
+ if (direrr < 0)
goto cleanup;
- }
if (nodeinfo->cpus && nodeinfo->nodes)
goto done;
--
1.9.2
5:09 p.m.
New subject: [libvirt] [PATCH 2/2] nodeinfo: use virDirRead API
On 04/20/2014 05:53 AM, Natanael Copa wrote:
This makes sure that errno is reset before readdir is called, even
if
the loop does a 'continue'.
This fixes issue with musl libc which sets errno on sscanf. The
following 'continue' makes the errno be set before calling readdir.
Signed-off-by: Natanael Copa <ncopa(a)alpinelinux.org>
---
src/nodeinfo.c | 29 ++++++++---------------------
1 file changed, 8 insertions(+), 21 deletions(-)
@@ -452,8 +453,7 @@ virNodeParseNode(const char *node,
/* enumerate sockets in the node */
CPU_ZERO(&sock_map);
- errno = 0;
- while ((cpudirent = readdir(cpudir))) {
+ while ((direrr = virDirRead(cpudir, &cpudirent, node)) == 0) {
Depends on whether we tweak return semantics of 0 vs 1 in patch 1. But
modulo that design decision (anyone else with an opinion?) the
conversion is sane. Are you planning on filling out the series to
include all other clients of readdir()?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:05 p.m.
On 04/20/2014 05:53 AM, Natanael Copa wrote:
Introduce a wrapper for readdir. This helps us make sure that we
always
set errno before calling readdir and it will make sure errors are
properly logged.
Signed-off-by: Natanael Copa <ncopa(a)alpinelinux.org>
---
src/util/virfile.c | 14 ++++++++++++++
src/util/virfile.h | 3 +++
2 files changed, 17 insertions(+)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 3eb2703..b54b9fd 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -2295,6 +2295,20 @@ virDirCreate(const char *path ATTRIBUTE_UNUSED,
}
#endif /* WIN32 */
+/* return 0 = success, 1 = end-of-dir and -1 = error */
This logic is a bit odd to reason about. I think 1 = success (returned
1 entry), 0 = end-of-dir (successfully returned 0 entries) may be a bit
easier to reason about.
With your construct, the loop looks like:
while (!(value = virReadDir(dir, &ent, name))) {
...
}
if (value < 0)
error
with my proposed return values, the loop looks like:
while ((value = virReadDir(dir, &ent, name)) > 0) {
...
}
if (value < 0)
error
Mine is slightly more typing, but seems a bit more intuitive. Anyone
else with an opinion?
+int virDirRead(DIR *dirp, struct dirent **ent, const char *dirname)
Many of our wrappers are named resembling what they wrap, as in
virReaddir(). On the other hand, we already have the virDir... prefix
for other actions, so I can live with this name.
+{
+ errno = 0;
+ *ent = readdir(dirp);
Due to this statement, both ent and dirp must be non-NULL pointers...
@@ -211,6 +212,8 @@ enum {
};
int virDirCreate(const char *path, mode_t mode, uid_t uid, gid_t gid,
unsigned int flags) ATTRIBUTE_RETURN_CHECK;
+int virDirRead(DIR *dirp, struct dirent **ent, const char *dirname);
...so I'd add:
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK.
I like how you made error reporting optional - pass a name to report the
error, pass NULL to suppress it while still getting an error indication.
There's also the point I raised about whether we should make the wrapper
function automatically skip . and ..; this would best be done by adding
a flags argument, where the default of 0 returns all entries, but
passing a non-zero flag can do filtering. I guess it still remains to
be seen how many callers would benefit from this. Also, your series
only touched one file to use the new paradigm; but ideally we'd add a
syntax check that enforces its use everywhere.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:31 p.m.
On 04/21/2014 04:05 PM, Eric Blake wrote:
On 04/20/2014 05:53 AM, Natanael Copa wrote:
> Introduce a wrapper for readdir. This helps us make sure that we always
> set errno before calling readdir and it will make sure errors are
> properly logged.
>
There's also the point I raised about whether we should make the
wrapper
function automatically skip . and ..; this would best be done by adding
a flags argument, where the default of 0 returns all entries, but
passing a non-zero flag can do filtering. I guess it still remains to
be seen how many callers would benefit from this. Also, your series
only touched one file to use the new paradigm; but ideally we'd add a
syntax check that enforces its use everywhere.
Something like this will make it easy to pick out the remaining spots in
the code that need conversion, and enforce the style in the future. You
can apply it now to find all the culprits, but rebase it to be the last
patch in the series to avoid bisection failures.
========
diff --git i/cfg.mk w/cfg.mk
index 8a444d6..3f4bba0 100644
--- i/cfg.mk
+++ w/cfg.mk
@@ -421,6 +421,12 @@ sc_prohibit_gethostname:
halt='use virGetHostname, not gethostname' \
$(_sc_search_regexp)
+sc_prohibit_readdir:
+ @prohibit='\breaddir *\(' \
+ exclude='exempt from syntax-check' \
+ halt='use virDirRead, not readdir' \
+ $(_sc_search_regexp)
+
sc_prohibit_gettext_noop:
@prohibit='gettext_noop *\(' \
halt='use N_, not gettext_noop' \
diff --git i/src/util/virfile.c w/src/util/virfile.c
index b54b9fd..54a1b25 100644
--- i/src/util/virfile.c
+++ w/src/util/virfile.c
@@ -2299,7 +2299,7 @@ virDirCreate(const char *path ATTRIBUTE_UNUSED,
int virDirRead(DIR *dirp, struct dirent **ent, const char *dirname)
{
errno = 0;
- *ent = readdir(dirp);
+ *ent = readdir(dirp); /* exempt from syntax-check */
if (!*ent && errno) {
if (dirname)
virReportSystemError(errno, _("Unable to read directory
'%s'"),
=========
For reference, here's what still needs cleaning up (I ran this before
applying your patch 2/2, so the three nodeinfo.c instances go away with
that one). Hmm, the secret_driver.c points out a false positive in the
syntax check, where we'd have to tweak the comment wording.
src/conf/domain_conf.c:18136: while ((entry = readdir(dir))) {
src/conf/network_conf.c:3151: while ((entry = readdir(dir))) {
src/conf/network_conf.c:3185: while ((entry = readdir(dir))) {
src/conf/nwfilter_conf.c:3108: while ((entry = readdir(dir))) {
src/conf/storage_conf.c:1874: while ((entry = readdir(dir))) {
src/nodeinfo.c:456: while ((cpudirent = readdir(cpudir))) {
src/nodeinfo.c:494: while ((cpudirent = readdir(cpudir))) {
src/nodeinfo.c:676: while ((nodedirent = readdir(nodedir))) {
src/openvz/openvz_conf.c:1109: while ((dent = readdir(dp))) {
src/parallels/parallels_storage.c:104: while ((ent = readdir(dir)) !=
NULL) {
src/parallels/parallels_storage.c:342: while ((ent = readdir(dir)) !=
NULL) {
src/qemu/qemu_driver.c:442: while ((entry = readdir(dir))) {
src/qemu/qemu_hostdev.c:98: while ((iommuGroup = readdir(iommuDir))) {
src/secret/secret_driver.c:487: while ((de = readdir(dir)) != NULL) {
src/secret/secret_driver.c:506: /* Ignore error reported by
readdir(), if any. It's better to keep the
src/storage/storage_backend.c:1610: while ((dent = readdir(dh)) !=
NULL) {
src/storage/storage_backend_fs.c:865: while ((ent = readdir(dir)) !=
NULL) {
src/storage/storage_backend_iscsi.c:110: while ((dirent =
readdir(sysdir))) {
src/storage/storage_backend_scsi.c:255: while ((block_dirent =
readdir(block_dir))) {
src/storage/storage_backend_scsi.c:336: while ((lun_dirent =
readdir(lun_dir))) {
src/storage/storage_backend_scsi.c:445: while ((lun_dirent =
readdir(devicedir))) {
src/util/vircgroup.c:3154: ent = readdir(grpdir);
src/util/vircgroup.c:3399: while ((ent = readdir(dp))) {
src/util/vircgroup.c:3724: while ((de = readdir(dh)) != NULL) {
src/util/virfile.c:611: while ((de = readdir(dh)) != NULL) {
src/util/virfile.c:784: while ((de = readdir(dh)) != NULL) {
src/util/virfile.c:920: while ((de = readdir(dh)) != NULL) {
src/util/virnetdevtap.c:108: while ((dp = readdir(dirp)) != NULL) {
src/util/virpci.c:448: while ((entry = readdir(dir))) {
src/util/virpci.c:1921: while ((ent = readdir(dir)) != NULL) {
src/util/virpci.c:1976: while ((errno = 0, ent = readdir(groupDir))
!= NULL) {
src/util/virpci.c:2641: while ((entry = readdir(dir))) {
src/util/virscsi.c:136: while ((entry = readdir(dir))) {
src/util/virscsi.c:181: while ((entry = readdir(dir))) {
src/util/virusb.c:149: while ((de = readdir(dir))) {
src/util/virutil.c:1870: while ((entry = readdir(dir))) {
src/util/virutil.c:1952: while ((entry = readdir(dir))) {
src/xen/xen_inotify.c:366: while ((ent = readdir(dh))) {
src/xen/xm_internal.c:330: while ((ent = readdir(dh))) {
maint.mk: use virDirRead, not readdir
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:16 a.m.
On Mon, 21 Apr 2014 16:05:28 -0600
Eric Blake <eblake(a)redhat.com> wrote:
On 04/20/2014 05:53 AM, Natanael Copa wrote:
...
> +/* return 0 = success, 1 = end-of-dir and -1 = error */
This logic is a bit odd to reason about. I think 1 = success (returned
1 entry), 0 = end-of-dir (successfully returned 0 entries) may be a bit
easier to reason about.
With your construct, the loop looks like:
while (!(value = virReadDir(dir, &ent, name))) {
...
}
if (value < 0)
error
with my proposed return values, the loop looks like:
while ((value = virReadDir(dir, &ent, name)) > 0) {
...
}
if (value < 0)
error
Mine is slightly more typing, but seems a bit more intuitive. Anyone
else with an opinion?
It was to be consistent with the current virDirCreate which returns 0
on success, like many of the posix functions. But its your code, and
you that will have to live with it. I don't have any strong opinion.
> +int virDirRead(DIR *dirp, struct dirent **ent, const char
*dirname)
Many of our wrappers are named resembling what they wrap, as in
virReaddir(). On the other hand, we already have the virDir... prefix
for other actions, so I can live with this name.
The idea was to group it in the same category as virDirCreate. Might be
you want virDirOpen, virDirClose, virDirRewind in future. And maybe
even move it out from virfile to virdir.
> +{
> + errno = 0;
> + *ent = readdir(dirp);
Due to this statement, both ent and dirp must be non-NULL pointers...
> @@ -211,6 +212,8 @@ enum {
> };
> int virDirCreate(const char *path, mode_t mode, uid_t uid, gid_t gid,
> unsigned int flags) ATTRIBUTE_RETURN_CHECK;
> +int virDirRead(DIR *dirp, struct dirent **ent, const char *dirname);
...so I'd add:
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK.
I'm not familiar with those. I'll lave that to you.
I like how you made error reporting optional - pass a name to report
the
error, pass NULL to suppress it while still getting an error indication.
There's also the point I raised about whether we should make the wrapper
function automatically skip . and ..; this would best be done by adding
a flags argument, where the default of 0 returns all entries, but
passing a non-zero flag can do filtering. I guess it still remains to
be seen how many callers would benefit from this.
I'll leave that to you. It now runs on musl libc so I'm done scratching
my itch ;)
Also, your series
only touched one file to use the new paradigm; but ideally we'd add a
syntax check that enforces its use everywhere.
I was hoping you would take care of it from here. You now know what the
problem is and you know your own code better than I do.
Thanks for the feedback.
-nc
3840
days inactive
3842
days old
5 comments
2 participants
participants (2)
-
Eric Blake -
Natanael Copa