Re: CVS commit: src/sys/kern

[Elad Efrat <elad%bsd.org.il@localhost>]

Mindaugas R. wrote:
> Elad Efrat <elad%bsd.org.il@localhost> wrote:
>> this isn't what I pointed out. I told you this change is wrong, and I
>> also asked who okay'd it. revert these two commits.
> Actually, you didn't pointed anything except "this is wrong".
> 
> specificdata_getspecific() can return NULL -> mount_getspecific() can return
> NULL -> fileassoc_table_lookup() can return NULL. RUN_ONCE in
> fileassoc_table_lookup() may also return NULL too.
> 
> I may be wrong, of course, but instead of screaming and chirping, maybe you
> can elaborate why it is wrong?
> 

I'm not "screaming and chirping". I sent you a mail off-list that said
the following:

        "who okay'd this commit? this is wrong. read the code."

you chose to ignore this and decided I pointed out a typo (?), so I had
to clarify.

first, if you are not 100% sure why what you committed is *right*, it is
enough for you to understand you need to revert it.

this is the relevant function:

  524: int
  525: fileassoc_file_delete(struct vnode *vp)
  526: {
  527:        struct fileassoc_table *tbl;
  528:        struct fileassoc_hash_entry *mhe;
  529:
  530:        mhe = fileassoc_file_lookup(vp, NULL);
  531:        if (mhe == NULL)
  532:                return (ENOENT);
  533:
  534:        file_free(mhe);
  535:
  536:        tbl = fileassoc_table_lookup(vp->v_mount);
  537:        if (tlb == NULL)
  538:                return (ENOENT);
  539:        --(tbl->hash_used); /* XXX gc? */
  540:
  541:        return (0);
  542: }

(note that lines 537-538 you added.)

the first thing that happens on entry to fileassoc_file_delete() is
fileassoc_file_lookup(). let's see:

  253: static struct fileassoc_hash_entry *
  254: fileassoc_file_lookup(struct vnode *vp, fhandle_t *hint)
  255: {
  256:        struct fileassoc_table *tbl;
  257:        struct fileassoc_hashhead *tble;
  258:        struct fileassoc_hash_entry *e;
  259:        size_t indx;
  260:        fhandle_t *th;
  261:        int error;
  262:
  263:        tbl = fileassoc_table_lookup(vp->v_mount);
  264:        if (tbl == NULL) {
  265:                return NULL;
  266:        }
  [...]

the only way fileassoc_table_lookup() can fail in
fileassoc_table_delete() is if there's a locking violation. since
fileassoc lacks locking altogether, your introduced workaround isn't a
real fix, but rather a way to silence a coverity "bug".

http://www.netbsd.org/cgi-bin/query-pr-single.pl?number=35351

this is a pr titled "fileassoc lacks locking" from 2007-01-01. unless
you are going to *fix* the problem, don't modify code you do not
understand.

and, still, you did not tell me who okay'd this commit. iiuc, you are
a rather new developer on the project (less than two months?) and as
such expected to have your sponsors okay your commits. so not only you
modified code you don't understand, you also didn't compile test and
didn't ask for approval for the change.

way to go. please revert.

-e.