netdev - Re: [PATCH v2 0/4] kselftest: add fixture parameters

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 16 Mar 2020 14:01:33 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     "Bird, Tim" <Tim.Bird@...y.com>,
        "shuah@...nel.org" <shuah@...nel.org>,
        "luto@...capital.net" <luto@...capital.net>,
        "wad@...omium.org" <wad@...omium.org>,
        "linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kernel-team@...com" <kernel-team@...com>
Subject: Re: [PATCH v2 0/4] kselftest: add fixture parameters

On Mon, Mar 16, 2020 at 01:04:16PM -0700, Jakub Kicinski wrote:
> Variant sounds good too, although the abbreviation would be VAR?
> Which isn't ideal. 
> 
> But I really don't care so whoever cares the most please speak up :P

Let's go with "variant" and just spell it out.

> > BTW - Fuego has a similar feature for naming a collection of test
> > parameters with specific values (if I understand this proposed
> > feature correctly).  Fuego's feature was named a long time ago
> > (incorrectly, I think) and it continues to bug me to this day.
> > It was named 'specs', and after giving it considerable thought
> > I've been meaning to change it to 'variants'.
> > 
> > Just a suggestion for consideration.  The fact that Fuego got this
> > wrong is what motivates my suggestion today.  You have to live
> > with this kind of stuff a long time. :-)
> > 
> > We ran into some issues in Fuego with this concept, that motivate
> > the comments below.  I'll use your 'instance' terminology in my comments
> > although the terminology is different in Fuego.
> > 
> > > Also a change in reporting:
> > > 
> > > 	struct __fixture_params_metadata no_param = { .name = "", };
> > > 
> > > Let's make ".name = NULL" here, and then we can detect instantiation:
> > > 
> > > 	printf("[ RUN      ] %s%s%s.%s\n", f->name, p->name ? "." : "",
> > > 				p->name ?: "", t->name);
> 
> Do I have to make it NULL or is it okay to test p->name[0] ?
> That way we can save one ternary operator from the litany..

I did consider Tim's idea of having them all say 'default', but since
the bulk of tests aren't going to have variants, I don't want to spam
the report with words I have to skip over.

And empty-check (instead of NULL) is fine by me.

> To me global.default.XYZ is a mouthful. so in my example (perhaps that
> should have been part of the cover letter) I got:
> 
> [ RUN      ] global.keysizes             <= non-fixture test
> [       OK ] global.keysizes             
> [ RUN      ] tls_basic.base_base         <= fixture: "tls_basic", no params
> [       OK ] tls_basic.base_base         
> [ RUN      ] tls12.sendfile              <= fixture: "tls", param: "12"
> [       OK ] tls12.sendfile                 
> [ RUN      ] tls13.sendfile              <= fixture: "tls", param: "13"
> [       OK ] tls13.sendfile                 (same fixture, diff param)
> 
> And users can start inserting underscores themselves if they really
> want. (For TLS I was considering different ciphers but they don't impact
> testing much.)

The reason I'd like a dot is just for lay-person grep-ability and
to avoid everyone needing to remember to add separator prefixes --
there should just be a common one. e.g.  searching for "tls13" in the
tree wouldn't find the test (since it's actually named "tls" and "13"
is separate places). (I mean, sure, searching for "tls" is also insane,
but I think I made my point.)

-Kees

-- 
Kees Cook