On Montag, 5. September 2022 10:51:10 CEST Linus Heckemann wrote:
> Hi all, thanks for your reviews.
>
> > @@ -4226,7 +4232,7 @@ int v9fs_device_realize_common(V9fsState *s, const
> > V9fsTransport *t,>
> > s->ctx.fmode = fse->fmode;
> > s->ctx.dmode = fse->dmode;
> >
> > - QSIMPLEQ_INIT(&s->fid_list);
> > + s->fids = g_hash_table_new(NULL, NULL);
> >
> > qemu_co_rwlock_init(&s->rename_lock);
> >
> > if (s->ops->init(&s->ctx, errp) < 0) {
>
> I noticed that the hash table may be leaked as is. I'll address this in
> the next submission.
>
> Philippe Mathieu-Daudé <f4...@amsat.org> writes:
> > [Style nitpicking]
>
> Applied these changes and will include them in the next version of the
> patch.
> Christian Schoenebeck <qemu_...@crudebyte.com> writes:
> > > @@ -317,12 +315,9 @@ static V9fsFidState *alloc_fid(V9fsState *s,
> > > int32_t
> > > fid) {
> > >
> > > V9fsFidState *f;
> > >
> > > - QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
> > > + if (g_hash_table_contains(s->fids, GINT_TO_POINTER(fid))) {
> > >
> > > /* If fid is already there return NULL */
> > >
> > > - BUG_ON(f->clunked);
> > > - if (f->fid == fid) {
> > > - return NULL;
> > > - }
> > > + return NULL;
> >
> > Probably retaining BUG_ON(f->clunked) here?
>
> I decided not to since this was a sanity check that was happening for
> _each_ fid, but only up to the one we were looking for. This seemed
> inconsistent and awkward to me, so I dropped it completely (and the
> invariant that no clunked fids remain in the table still seems to hold
> -- it's fairly trivial to check, in that the clunked flag is only set
> in two places, both of which also remove the map entry). My preference
> would be to leave it out, but I'd also be fine with restoring it for
> just the one we're looking for, or maybe moving the check to when we're
> iterating over the whole table, e.g. in v9fs_reclaim_fd. Thoughts?
Yeah, I think you are right, it would feel odd. Just drop BUG_ON() for now.
> > > @@ -424,12 +419,11 @@ static V9fsFidState *clunk_fid(V9fsState *s,
> > > int32_t
> > > fid) {
> > >
> > > V9fsFidState *fidp;
> > >
> > > - QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) {
> > > - if (fidp->fid == fid) {
> > > - QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next);
> > > - fidp->clunked = true;
> > > - return fidp;
> > > - }
> > > + fidp = g_hash_table_lookup(s->fids, GINT_TO_POINTER(fid));
> > > + if (fidp) {
> > > + g_hash_table_remove(s->fids, GINT_TO_POINTER(fid));
> > > + fidp->clunked = true;
> > > + return fidp;
> >
> > We can't get rid of the double lookup here, can we? Surprisingly I don't
> > find a lookup function on the iterator based API.
>
> It seems you're not the only one who had that idea:
> https://gitlab.gnome.org/GNOME/glib/-/issues/613
>
> In this case, I think an extended remove function which returns the
> values that were present would be even nicer. But neither exists at this
> time (and that issue is pretty old), I guess we're stuck with this for
> now.
Well, all we would need was such a proposed
g_hash_table_lookup_iter(table,key,iter) function. I just had a quick look at
ghash.c and it looks like that would actually be straightforward to add as the
iterator structure takes the same direct array index as the already existing
g_hash_table_lookup() function. But anyway, that's the current situation, so
be it.
> Daniel P. Berrangé writes:
> > In $SUBJECT it is called GHashTable, not GHashMap
>
> Indeed, good catch. Will fix in the next version.
>
> Linus
