On 12/29/2014 10:14 PM, sfel...@gmail.com wrote: > From: Scott Feldman <sfel...@gmail.com>
[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
