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

[Scott Feldman]

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?

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.

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

-scott