Re: [PATCH] 9pfs: use GHashMap for fid table

[Linus Heckemann <git@sphalerite.org>]

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é <f4bug@amsat.org> writes:
> [Style nitpicking]

Applied these changes and will include them in the next version of the patch.

Christian Schoenebeck <qemu_oss@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?

> > @@ -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.


Daniel P. Berrangé writes:
> In $SUBJECT it is called GHashTable, not GHashMap

Indeed, good catch. Will fix in the next version.

Linus