Re: [PATCH v6 3/5] 9pfs: add new function v9fs_co_readdir_many()

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Montag, 4. Mai 2020 11:18:34 CEST Greg Kurz wrote:
> > > > > > +        memcpy(e->dent, dent, sizeof(struct dirent));
> > > > > > +
> > > > > > +        /* perform a full stat() for directory entry if requested
> > > > > > by
> > > > > > caller */ +        if (dostat) {
> > > > > > +            err = s->ops->name_to_path(
> > > > > > +                &s->ctx, &fidp->path, dent->d_name, &path
> > > > > > +            );
> > > > > > +            if (err < 0) {
> > > > > > 
> > > > > >                  err = -errno;
> > > > > > 
> > > > > > -            } else {
> > > > > > -                *dent = entry;
> > > > > > -                err = 0;
> > > > > > +                break;
> > > > > 
> > > > > ... but we're erroring out there and it seems that we're leaking
> > > > > all the entries that have been allocated so far.
> > > > 
> > > > No, they are not leaking actually.
> > > > 
> > > > You are right that they are not deallocated in do_readdir_many(), but
> > > > that's intentional: in the new implementation of v9fs_do_readdir() you
> > > > see that v9fs_free_dirents(entries) is *always* called at the very end
> > > > of
> > > > the function, no matter if success or any error. That's one of the
> > > > measures to simplify overall code as much as possible.
> > > 
> > > Hmm... I still don't quite like the idea of having an erroring function
> > > asking for extra cleanup. I suggest you come up with an idem-potent
> > > version
> > > of v9fs_free_dirents(), move it to codir.c (I also prefer locality of
> > > calls
> > > to g_malloc and g_free in the same unit), make it extern and call it
> > > both on the error path of v9fs_co_readdir_many() and in
> > > v9fs_do_readdir().
> > 
> > I understand your position of course, but I still won't find that to be a
> > good move.
> > 
> > My veto here has a reason: your requested change would prevent an
> > application that I had in mind for future purpose actually: Allowing
> > "greedy" fetching
> Are you telling that this series has some kind of hidden agenda related to
> a possible future change ?!?

readdir_many() is written intended as general purpose directory retrieval 
function, that is for other purposes in future in mind, yes.

What I don't do is adding code which is not explicitly needed right now of 
course. That would not make sense and would make code unnecessarily bloated 
and of course too complicated (e.g. readdir_many() is currently simply 
directly calling v9fs_readdir_response_size() to decide whether to terminate 
the loop instead of taking some complicated general-purpose loop end 
"predicate" structure or callback as function argument).

But when it comes to the structure of the code that I have to add NOW, then I 
indeed take potential future changes into account, yes! And this applies 
specifically to the two changes you requested here inside readdir_many(). 
Because I already know, I would need to revert those 2 changes that you 
requested later on. And I don't see any issue whatsover retaining the current 
version concerning those 2.

Best regards,
Christian Schoenebeck