Re: [Qemu-devel] [PATCH v2 07/10] rocker: add new rocker switch device

[Stefan Hajnoczi]

On Tue, Jan 06, 2015 at 08:45:44AM -0800, Scott Feldman wrote:
> On Tue, Jan 6, 2015 at 7:12 AM, Stefan Hajnoczi <address@hidden> wrote:
> > On Mon, Jan 05, 2015 at 06:24:58PM -0800, address@hidden wrote:
> >> From: Scott Feldman <address@hidden>
> >>
> >> Rocker is a simulated ethernet switch device.  The device supports up to 62
> >> front-panel ports and supports L2 switching and L3 routing functions, as 
> >> well
> >> as L2/L3/L4 ACLs.  The device presents a single PCI device for each switch,
> >> with a memory-mapped register space for device driver access.
> >>
> >> Rocker device is invoked with -device, for example a 4-port switch:
> >>
> >>   -device rocker,name=sw1,len-ports=4,ports[0]=dev0,ports[1]=dev1, \
> >>          ports[2]=dev2,ports[3]=dev3
> >>
> >> Each port is a netdev and can be paired with using -netdev id=<port name>.
> >
> > This design looks good, it fits the QEMU network subsystem.
> >
> > Please follow QEMU coding style, for example, using typedefs for structs
> > instead of "struct tag".  Details are in ./HACKING, ./CODING_STYLE, and
> > you can scan patches with QEMU's scripts/checkpatch.pl.
> 
> The patches are already scripts/checkpatch.pl clean.
> 
> And we did follow the HACKING and CODING_STYLE guidelines, with the
> exception of typedefs for structs.  Did you spot anything else
> out-of-compliance?

No, just the lack of typedef struct caught my eye.

> On typedefs for structs, there are plenty of examples in Qemu of not
> following that rule.  Perhaps this rule can be enforced by
> checkpatch.pl?  Personally, I feel that typedef on a struct makes the
> code harder to read as it obfuscate the type.  For example, typedef
> union {...} foo or typedef struct {...} foo or typedef enum {...} foo:
> there is no way to tell with foo bar what bar is, at a glance, whereas
> union foo bar or struct foo bar or enum foo bar is clear.

It seems checkpatch.pl doesn't enforce the rule.

There is old code that doesn't follow the coding standard, but new code
should.

> We can make the typedef change for v3 if it's a hard requirement for 
> inclusion.

Thank you!

pgpTuUAd0QEJi.pgp
Description: PGP signature