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@xxxxxxxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list