Re: [PATCH v5] 9pfs: use GHashTable for fid table

[Linus Heckemann <git@sphalerite.org>]

Linus Heckemann <git@sphalerite.org> writes:
>  static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
>  {
>      V9fsState *s = pdu->s;
>      V9fsFidState *fidp;
> +    GList *freeing;
> +    /* Get a list of all the values (fid states) in the table, which we then... */
> +    g_autoptr(GList) fids = g_hash_table_get_values(s->fids);
>  
> -    /* Free all fids */
> -    while (!QSIMPLEQ_EMPTY(&s->fid_list)) {
> -        /* Get fid */
> -        fidp = QSIMPLEQ_FIRST(&s->fid_list);
> -        fidp->ref++;
> +    /* ... remove from the table, taking over ownership. */
> +    g_hash_table_steal_all(s->fids);
>  
> -        /* Clunk fid */
> -        QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next);
> +    /*
> +     * This allows us to release our references to them asynchronously without
> +     * iterating over the hash table and risking iterator invalidation
> +     * through concurrent modifications.
> +     */
> +    for (freeing = fids; freeing; freeing = freeing->next) {
> +        fidp = freeing->data;
> +        fidp->ref++;
>          fidp->clunked = true;
> -
>          put_fid(pdu, fidp);
>      }
>  }

I'm not sure if this implementation is correct. I'm concerned that it
may result in dangling references, but haven't been able to find a
client that will send the TVERSION request on a connection that's
already been used in other ways, as opposed to when the connection is
first established. I suspect this will be very rare in general, so it
might be good to have a test case somewhere.