On 2019-07-22 7:50 a.m., Christoph Hellwig wrote:
On Mon, Jul 22, 2019 at 08:22:30AM +0200, Hannes Reinecke wrote:
Gcc-9 complains for a memset across pointer boundaries, which happens
as the code tries to allocate a flexible array on the stack.
Turns out we cannot do this without relying on gcc-isms, so
with this patch we'll embed the fc_rport_priv structure into
fcoe_rport, can use the normal 'container_of' outcast, and
will only have to do a memset over one structure.
This looks mostly good, but:
I think the subject and changelog are a bit odd. What you do here
is to change that way how the private data is allocated by embeddeding
the fc_rport_priv structure into the private data, so that should be
the focus of the description. That this was triggered by the memset
warning might be worth mentioning, but probably only after explaining
what you did.
@@ -2738,10 +2736,7 @@ static int fcoe_ctlr_vn_recv(struct fcoe_ctlr *fip, struct sk_buff *skb)
{
struct fip_header *fiph;
enum fip_vn2vn_subcode sub;
- struct {
- struct fc_rport_priv rdata;
- struct fcoe_rport frport;
- } buf;
+ struct fcoe_rport buf;
Wouldn't rport or frport be a better name for this variable?
fiph = (struct fip_header *)skb->data;
@@ -2757,7 +2752,8 @@ static int fcoe_ctlr_vn_recv(struct fcoe_ctlr *fip, struct sk_buff *skb)
goto drop;
}
- rc = fcoe_ctlr_vn_parse(fip, skb, &buf.rdata);
+ memset(&buf, 0, sizeof(buf));
Instead of the memset you could do an initialization at declaration
time:
struct fcoe_rport rport = { };
https://en.cppreference.com/w/c/language/struct_initialization
"When initializing an object of struct or union type, the initializer must
be a non-empty, brace-enclosed, comma-separated list of initializers for
the members:"
Hmmm, "non-empty", is that a GNU extension?
However it is good C++11, so if that is where we a moving, great.
Doug Gilbert
- struct {
- struct fc_rport_priv rdata;
- struct fcoe_rport frport;
- } buf;
+ struct fcoe_rport buf;
int rc;
fiph = (struct fip_header *)skb->data;
sub = fiph->fip_subcode;
- rc = fcoe_ctlr_vlan_parse(fip, skb, &buf.rdata);
+ memset(&buf, 0, sizeof(buf));
Same two comments apply here.
