On Mon, 21 Apr 2014 16:05:28 -0600
Eric Blake <eblake@xxxxxxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list