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@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