Re: [libvirt] [PATCH 1/2] util: introduce virDirRead wrapper for readdir

On 04/20/2014 05:53 AM, Natanael Copa wrote: 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? 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. Due to this statement, both ent and dirp must be non-NULL pointers... ...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