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

[Christian Schoenebeck]

On Dienstag, 27. September 2022 17:14:57 CEST Linus Heckemann wrote:
> 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.

Always welcome! :)
https://wiki.qemu.org/Documentation/9p#Test_Cases

If you do, then please add the test as a separate patch.

Best regards,
Christian Schoenebeck