Hi Linus,
Thanks for this promising change !
On Mon, 05 Sep 2022 10:51:10 +0200
Linus Heckemann <g...@sphalerite.org> 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.
>
Pay attention that this rollback should be added to
v9fs_device_unrealize_common() which is assumed to be
idempotent and is already called on the error path of
v9fs_device_realize_common().
>
> 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?
>
Well... finding at least one clunked fid state in this table is
definitely a bug so I'll keep the BUG_ON() anyway.
> > > @@ -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.
g_hash_table_steal_extended() [1] actually allows to do just that. Since the
hash table is allocated with g_hash_table_new() and doesn't care for destroy
functions, the code change would be something like:
@@ -424,12 +419,10 @@ 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;
- }
+ if (g_hash_table_steal_extended(s->fids, GINT_TO_POINTER(fid), NULL,
+ (gpointer *)&fidp)) {
+ fidp->clunked = true;
+ return fidp;
}
return NULL;
}
[1]
https://developer-old.gnome.org/glib/stable/glib-Hash-Tables.html#g-hash-table-steal-extended
Cheers,
--
Greg
>
>
> Daniel P. Berrangé writes:
> > In $SUBJECT it is called GHashTable, not GHashMap
>
> Indeed, good catch. Will fix in the next version.
>
> Linus
