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
