Re: Fileassoc locking

[Mindaugas Rasiukevicius <rmind%netbsd.org@localhost>]

Elad Efrat <elad%NetBSD.org@localhost> wrote:
> > - fileassoc_table_lookup() or its uses are missing locking (reference does
> > Ânot provide exclusion). ÂThus, fileassoc_table does not need
> > ref-counting, unless for workarounds in fileassoc_table_run(), see below.
> > ÂUse of RW-lock would fit in this case.
> 
> Why do we need to hold the table exclusively in fileassoc_table_lookup()?

See below.

> > - fileassoc_file_lookup() is wrong: fileassoc_table should be locked when
> > Âperforming lookup, it does not need ref-counting. ÂAlso, no need to hold
> > Âa reference on each fileassoc_file entry, reference must be held and kept
> > Âfor caller on entry which was found.
> 
> Hmm... Why should the table be locked? If we hold a reference, and
> modifiers of the table will not modify it until they hold it
> exclusively, why should we lock it...? Also, I'm holding a reference
> to the file because we use it in the comparisons (of the file handle
> etc.) -- this is to prevent it from being modified while in use.

And here you duplicate basically the same what rwlock or mutex provides
for you (i.e. exclusion, just hardcoded).  There is no reason for that.

Same point with other mentioned cases, so I will skip them.

> As for returning the entry with a reference -- I'll do that once the
> rest of the locking is "done" because it'll require changes to users
> of fileassoc(9)...

What needs fileassoc_release()?

> > - Why fileassoc_file_add() and fileassoc_file_delete() are waiting for
> > zero references? ÂIt does not look to me that fileassoc_file requires
> > condvar, e.g. last reference holder can free it. Â Also,
> > fileassoc_file_add() checks for duplicates - I think caller (of API)
> > should do that.
> 
> <...>
> 
> As for your suggestion that the KPI user should check for duplicates
> -- note that to the KPI user, a "file" is really a combination of
> "file" and "assoc," which is properly detected in fileassoc_add()
> where EEXIST is returned if you're trying to add a combination that
> already exists. Otherwise, the "file" part is used internally, so the
> KPI user can't (and shouldn't) check whether it exists or not.

Why not?  Caller can use fileassoc_lookup().  And if it does, then
fileassoc_add() performs unnecessary lookup.

-- 
Mindaugas