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

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140421/9c3d1ced/attachment-0001.sig>