Re: [Qemu-devel] [PATCH 08/10] qmp: add rocker device support

[Eric Blake]

On 12/29/2014 10:14 PM, address@hidden wrote:
> From: Scott Feldman <address@hidden>

[your message came through as a top-level thread instead of in-reply-to
the 0/10 cover letter; please see if you can fix that before the next
submission]

> 
> Add QMP/HMP support for rocker devices.  This is mostly for debugging purposes
> to see inside the device's tables and port configurations.  Some examples:
> 

In this mail, I'll review just the QMP interface portion:

> +++ b/qapi-schema.json
> @@ -3515,3 +3515,54 @@
>  # Since: 2.1
>  ##
>  { 'command': 'rtc-reset-reinjection' }
> +
> +{ 'type': 'Rocker',
> +  'data': { 'name': 'str', 'id': 'uint64', 'ports': 'uint32' } }

Please add documentation for each new type and each member of that type,
including mention of which qemu release will first contain it.

> +
> +{ 'command': 'rocker',
> +  'data': { 'name': 'str' },
> +  'returns': 'Rocker' }
> +
> +{ 'type': 'RockerPort',
> +  'data': { 'name': 'str', 'enabled': 'bool', 'link_up': 'bool',
> +            'speed': 'uint32', 'duplex': 'uint8', 'autoneg': 'uint8' } }

Why is 'duplex' a 'uint8'?  Shouldn't it instead be an enum type?
Similar for 'autoneg'.

> +
> +{ 'command': 'rocker-ports',
> +  'data': { 'name': 'str' },
> +  'returns': ['RockerPort'] }
> +
> +{ 'type': 'RockerOfDpaFlowKey',
> +  'data' : { 'priority': 'uint32', 'tbl_id': 'uint32', '*in_pport': 'uint32',

Please use '-' rather than '_' in the spelling of new commands.

> +             '*tunnel_id': 'uint32', '*vlan_id': 'uint16',
> +             '*eth_type': 'uint16', '*eth_src': 'str', '*eth_dst': 'str',
> +             '*ip_proto': 'uint8', '*ip_tos': 'uint8', '*ip_dst': 'str' } }

Is 'str' the right type for IP addresses, or should it be a more
specific type?

> +
> +{ 'type': 'RockerOfDpaFlowMask',
> +  'data' : { '*in_pport': 'uint32', '*tunnel_id': 'uint32',
> +             '*vlan_id': 'uint16', '*eth_src': 'str', '*eth_dst': 'str',
> +             '*ip_proto': 'uint8', '*ip_tos': 'uint8' } }
> +
> +{ 'type': 'RockerOfDpaFlowAction',
> +  'data' : { '*goto_tbl': 'uint32', '*group_id': 'uint32',
> +             '*tun_log_pport': 'uint32', '*vlan_id': 'uint16',
> +             '*new_vlan_id': 'uint16', '*out_pport': 'uint32' } }
> +
> +{ 'type': 'RockerOfDpaFlow',
> +  'data': { 'cookie': 'uint64', 'hits': 'uint64', 'key': 
> 'RockerOfDpaFlowKey',
> +            'mask': 'RockerOfDpaFlowMask', 'action': 'RockerOfDpaFlowAction' 
> } }
> +
> +{ 'command': 'rocker-of-dpa-flows',
> +  'data': { 'name': 'str', '*tbl_id': 'uint32' },
> +  'returns': ['RockerOfDpaFlow'] }

Is the idea here that omitting 'tbl_id' (should be spelled 'tbl-id')
will give you an array of all table entries, while specifying an id will
give you an array of just the one requested entry?

> +
> +{ 'type': 'RockerOfDpaGroup',
> +  'data': { 'id': 'uint32',  'type': 'uint8', '*vlan_id': 'uint16',
> +            '*pport': 'uint32', '*index': 'uint32', '*out_pport': 'uint32',
> +            '*group_id': 'uint32', '*set_vlan_id': 'uint16',
> +            '*pop_vlan': 'uint8', '*group_ids': ['uint32'],
> +            '*set_eth_src': 'str', '*set_eth_dst': 'str',
> +            '*ttl_check': 'uint8' } }
> +
> +{ 'command': 'rocker-of-dpa-groups',
> +  'data': { 'name': 'str', '*type': 'uint8' },
> +  'returns': ['RockerOfDpaGroup'] }

Needs documentation before I can tell for sure, but based on just the
qapi specification, I think it will be reasonable once you fix naming
conventions.

> +++ b/qmp-commands.hx
> @@ -3860,3 +3860,27 @@ Move mouse pointer to absolute coordinates (20000, 
> 400).
>  <- { "return": {} }
>  
>  EQMP
> +
> +    {
> +        .name       = "rocker",
> +        .args_type  = "name:s",
> +        .mhandler.cmd_new = qmp_marshal_input_rocker,
> +    },
> +

Could you also provide example exchanges for each command added (what
the user passes in, and what qemu responds)?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature