linux-kernel - Re: [PATCH v2 0/2] remove workarounds for gcc bug wrt unnamed fields in initializers

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]

Message-ID: <BYAPR04MB49655BD99E428B66EBB831E486950@BYAPR04MB4965.namprd04.prod.outlook.com>
Date:   Wed, 24 Jun 2020 22:40:21 +0000
From:   Chaitanya Kulkarni <Chaitanya.Kulkarni@....com>
To:     Christoph Hellwig <hch@....de>, Keith Busch <kbusch@...nel.org>,
        Sagi Grimberg <sagi@...mberg.me>
CC:     Niklas Cassel <Niklas.Cassel@....com>, Jens Axboe <axboe@...com>,
        "linux-nvme@...ts.infradead.org" <linux-nvme@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 0/2] remove workarounds for gcc bug wrt unnamed fields
 in initializers

Christoph, Sagi and Keith,

On 6/24/20 9:44 AM, Christoph Hellwig wrote:
> This looks good to me, but I'd rather wait a few releases to
> avoid too mush backporting pain.
> 

Here is a summary, for longer explanation please have a look at the
end [1] :-

Pros:
1. Code looks uniform and follows strict policy.

Cons:
1. Adds a tab + more char [1] which can lead to line breaks and that can
    be avoided without following declare-init pattern, less bugs and
    no pressure to fit the initializer in ~72 char given that we do have
    some long names and who knows what is in the future.
2. Issue with older version can lead to adding additional braces which
    does not look good.
3. Writing a new code becomes inflexible and pressure to fit initializer
    will not allow users to use meaningful names in the nested structures
    and anon unions.
4. Future patches will be needed for backward compatibility.

Also code is perfectly readable as it is so why change ?

If everyone is okay with above cons I'm fine adding this.

Regards,
Chaitanya

[1] Explanation :-

I'm not against unifying the code. This will enforce struct 
initialization to be done at the time of declaration and is inflexible 
given that we have different transports and meaningful structure names.
Also, no one knows how many new structures will be coming since protocol 
still has a room for improvement.

Consider following :-

e.g. 1

static void nvme_xxx_func()
{
	struct nvme_XXX_YYY_ZZZ abcde {
		.member1  = line of the initializer calculation = X,
		.member2  = AAAAAAAAAAAAA + BBBBBBBBB + CCCCCC + DDDD +
			    EEEEE,
		.member3  = AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAa,
	}
}

e.g. 2

 From code :-
+	struct rdma_conn_param  param = {
+		.rnr_retry_count = 7,
+		.flow_control = 1,
+		.initiator_depth = min_t(u8, p->initiator_depth,
+	here ->>>queue->dev->device->attrs.max_qp_init_rd_atom),
+		.private_data = &priv,
+		.private_data_len = sizeof(priv),
+	};

In above case (e.g.1, e.g.2) we loose 8 character = 1 tab for every 
declaration-initialization, now if we have a member to be initialized 
with complex calculations then it comes down to the next line and again 
we loose 8 char of tab + (number of characters = name) of the member 
(member2 in nvme_xx_func()) and whole things looks ugly, in contrast if 
we do it outside of the declaration we still get 8 more characters 
before we reach line limit. With 80 char limit we should avoid line 
breaks and tabs as and when possible, this policy goes against it.