linux-kernel - 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] [day] [month] [year] [list]

Message-ID: <20200316142702.20497d0b@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 16 Mar 2020 14:27:02 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Kees Cook <keescook@...omium.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, 16 Mar 2020 14:01:33 -0700 Kees Cook wrote:
> 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.)

Ack, can't argue with grep-ability :)