Message ID | 20120302025554.GA13493@joana |
---|---|
State | Rejected, archived |
Delegated to: | David Miller |
Headers | show |
From: Gustavo Padovan <gustavo@padovan.org> Date: Thu, 1 Mar 2012 23:55:55 -0300 > - I'm pulling directly to you because due to the coding style issues that > happened some days ago. We decided, after talk to John, to pull directly to > you, so you can take a look before pull. I personally looked to the whole > diff of this pull request and couldn't spot anything that doesn't seem > specified in CodingStyle. For the members of "struct inquiry_cache" the tabbing has been all messed up by commit 561aafbcb2e3f8fee11d3781f866c7b4c4f93a28. Instead of all of the member names being nicely aligned using tabs it now looks like (after all the commits it's now called "struct discovery_state"): int type; enum { DISCOVERY_STOPPED, DISCOVERY_STARTING, DISCOVERY_FINDING, DISCOVERY_RESOLVING, DISCOVERY_STOPPING, } state; struct list_head all; /* All devices found during inquiry */ struct list_head unknown; /* Name state not known */ struct list_head resolve; /* Name needs to be resolved */ __u32 timestamp; instead of something more reasonable like: int type; enum { DISCOVERY_STOPPED, DISCOVERY_STARTING, DISCOVERY_FINDING, DISCOVERY_RESOLVING, DISCOVERY_STOPPING, } state; struct list_head all; /* All devices found during inquiry */ struct list_head unknown; /* Name state not known */ struct list_head resolve; /* Name needs to be resolved */ __u32 timestamp; The person editing this had to go out of their way to make things like this way. This was only three commits into your tree, therefore I'm not very optimistic to be honest with you. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Miller <davem@davemloft.net> Date: Thu, 01 Mar 2012 22:16:43 -0500 (EST) > This was only three commits into your tree, therefore I'm not very > optimistic to be honest with you. In the very next commit, 3175405b906a85ed2bad21e09c444266e4a05a8e we end up with: mgmt_device_found(hdev, &info->bdaddr, ACL_LINK, 0x00, info->dev_class, 0, 1, NULL); which after all the rest of the commits still looks like: mgmt_device_found(hdev, &info->bdaddr, ACL_LINK, 0x00, info->dev_class, 0, !name_known, ssp, NULL, 0); You have to make those arguments line up properly: mgmt_device_found(hdev, &info->bdaddr, ACL_LINK, 0x00, info->dev_class, 0, !name_known, ssp, NULL, 0); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Miller <davem@davemloft.net> Date: Thu, 01 Mar 2012 22:23:16 -0500 (EST) > From: David Miller <davem@davemloft.net> > Date: Thu, 01 Mar 2012 22:16:43 -0500 (EST) > >> This was only three commits into your tree, therefore I'm not very >> optimistic to be honest with you. > > In the very next commit, 3175405b906a85ed2bad21e09c444266e4a05a8e we end > up with: And then 2 commits later in ff9ef5787046c3fd20cf9f7ca1cd70260c1eedb9: + if (hdev->discovery.state != DISCOVERY_STOPPED) { + err = cmd_status(sk, index, MGMT_OP_START_DISCOVERY, + MGMT_STATUS_BUSY); + goto failed; + } it must be isntead: err = cmd_status(sk, index, MGMT_OP_START_DISCOVERY, MGMT_STATUS_BUSY); And that's pretty much as far as I'm willing to go. You know what really pisses me off? This is the one issue I made a huge stink about last week, arguments to function calls and prototypes lining up properl. And it's in ever 2nd or 3rd commit in this pull request. This means you really didn't check or you really don't care. Come back when you've audited this whole thing properly and honestly. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2012-03-01 at 22:26 -0500, David Miller wrote: > You know what really pisses me off? This is the one issue I made a huge > stink about last week, arguments to function calls and prototypes lining > up properl. And it's in ever 2nd or 3rd commit in this pull request. [] > Come back when you've audited this whole thing properly and honestly. I submitted a patch to checkpatch for a --strict rule for this argument alignment. https://lkml.org/lkml/2012/2/29/644 Are there any other tests you think should be added with --strict? Some I think possible are: o no space after cast struct foo *bar = (struct foo *) baz; sb: struct foo *bar = (struct foo *)baz; o don't start comments with /*$ (perhaps only if a blank line proceeds the comment) /* * multiline comment * ... */ sb: /* multiline comment * ... */ o coalesce formats pr_<level>("format part 1 " "format part 2\n", ...) sb: pr_<level>("format part 1 format part 2\n", ...); o symmetric brace standardization if (foo) { bar; baz; } else bar; sb: if (foo) { bar; baz; } else { bar; } Any others? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dave, * David Miller <davem@davemloft.net> [2012-03-01 22:26:04 -0500]: > From: David Miller <davem@davemloft.net> > Date: Thu, 01 Mar 2012 22:23:16 -0500 (EST) > > > From: David Miller <davem@davemloft.net> > > Date: Thu, 01 Mar 2012 22:16:43 -0500 (EST) > > > >> This was only three commits into your tree, therefore I'm not very > >> optimistic to be honest with you. > > > > In the very next commit, 3175405b906a85ed2bad21e09c444266e4a05a8e we end > > up with: > > And then 2 commits later in ff9ef5787046c3fd20cf9f7ca1cd70260c1eedb9: > > + if (hdev->discovery.state != DISCOVERY_STOPPED) { > + err = cmd_status(sk, index, MGMT_OP_START_DISCOVERY, > + MGMT_STATUS_BUSY); > + goto failed; > + } > > it must be isntead: > > err = cmd_status(sk, index, MGMT_OP_START_DISCOVERY, > MGMT_STATUS_BUSY); > > And that's pretty much as far as I'm willing to go. > > You know what really pisses me off? This is the one issue I made a huge > stink about last week, arguments to function calls and prototypes lining > up properl. And it's in ever 2nd or 3rd commit in this pull request. The styles in these patches is the same style we've been using in Bluetooth code for more than 10 years and no one ever complained to us and we are the only subsystem under Net doing something like this. I could find some examples over the net code in a quick look. I can see only two options here, leave this as is since this is coding style we've been using since beginning or change the whole Bluetooth subsystem to work like you said. In other words, are you telling me to make a patch to fix this issue in the whole Bluetooth subsystem and not only in this pull request diff? Otherwise we are putting the Bluetooth code in a inconsistent state with two different coding styles. Is that what you want, an inconsistent state? I'd be ok with changing the whole subsystem's style if you want. Gustavo -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Gustavo Padovan <padovan@profusion.mobi> Date: Fri, 2 Mar 2012 13:37:16 -0300 > are you telling me to make a patch to fix this issue in the whole > Bluetooth subsystem I'm saying get it right in new code changes. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html