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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 12050 bytes --]

On 01/22/2015 01:03 AM, sfeldma@gmail.com wrote:
> From: Scott Feldman <sfeldma@gmail.com>
> 
> 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:
> 

QMP interface review:

> +++ b/qapi-schema.json
> @@ -3523,3 +3523,6 @@
>  # Since: 2.1
>  ##
>  { 'command': 'rtc-reset-reinjection' }
> +
> +# Rocker ethernet network switch
> +{ 'include': 'qapi/rocker.json' }
> diff --git a/qapi/rocker.json b/qapi/rocker.json
> new file mode 100644
> index 0000000..326c6c7
> --- /dev/null
> +++ b/qapi/rocker.json
> @@ -0,0 +1,259 @@
> +##
> +# @Rocker:
> +#
> +# Rocker switch information.
> +#
> +# @name: switch name
> +#
> +# @id: switch ID
> +#
> +# @ports: number of front-panel ports
> +##

Missing a 'Since: 2.3' designation.

> +{ 'type': 'RockerSwitch',
> +  'data': { 'name': 'str', 'id': 'uint64', 'ports': 'uint32' } }
> +
> +##
> +# @rocker:
> +#
> +# Return rocker switch information.
> +#
> +# Returns: @Rocker information
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'rocker',
> +  'data': { 'name': 'str' },
> +  'returns': 'RockerSwitch' }

Should this command be named 'query-rocker', as it is used for queries?
 Should the 'name' argument be optional, and the output be an array (all
rocker devices, rather than just a given rocker name lookup)?

> +
> +##
> +# @RockerPortDuplex:
> +#
> +# An eumeration of port duplex states.
> +#
> +# @half: half duplex
> +#
> +# @full: full duplex
> +##

Missing a 'Since: 2.3' designation.

> +{ 'enum': 'RockerPortDuplex', 'data': [ 'half', 'full' ] }
> +
> +##
> +# @RockerPortAutoneg:
> +#
> +# An eumeration of port autoneg states.
> +#
> +# @off: autoneg is off
> +#
> +# @on: autoneg is on
> +##

Missing a 'Since: 2.3' designation.

> +{ 'enum': 'RockerPortAutoneg', 'data': [ 'off', 'on' ] }
> +
> +##
> +# @RockerPort:
> +#
> +# Rocker switch port information.
> +#
> +# @name: port name
> +#
> +# @enabled: port is enabled for I/O
> +#
> +# @link-up: physical link is UP on port
> +#
> +# @speed: port link speed in Mbps
> +#
> +# @duplex: port link duplex
> +#
> +# @autoneg: port link autoneg
> +##

Missing a 'Since: 2.3' designation.

> +{ 'type': 'RockerPort',
> +  'data': { 'name': 'str', 'enabled': 'bool', 'link-up': 'bool',
> +            'speed': 'uint32', 'duplex': 'RockerPortDuplex',
> +            'autoneg': 'RockerPortAutoneg' } }
> +
> +##
> +# @rocker-ports:
> +#
> +# Return rocker switch information.
> +#
> +# Returns: @Rocker information
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'rocker-ports',

Should this be named 'query-rocker-ports'?  Should the port information
be returned as part of the more generic 'rocker' command rather than
having to do a two-stage query (what are my rocker devices, then for
each device what are the ports)?

> +  'data': { 'name': 'str' },
> +  'returns': ['RockerPort'] }
> +
> +##
> +# @RockerOfDpaFlowKey:
> +#
> +# Rocker switch OF-DPA flow key
> +#
> +# @priority: key priority, 0 being lowest priority
> +#
> +# @tbl-id: flow table ID
> +#
> +# @in-pport: physical input port
> +#
> +# @tunnel-id: tunnel ID
> +#
> +# @vlan-id: VLAN ID
> +#
> +# @eth-type: Ethernet header type
> +#
> +# @eth-src: Ethernet header source MAC address
> +#
> +# @eth-dst: Ethernet header destination MAC address
> +#
> +# @ip-proto: IP Header protocol field
> +#
> +# @ip-tos: IP header TOS field
> +#
> +# @ip-dst: IP header destination address
> +##

Missing a 'Since: 2.3' designation.

> +{ 'type': 'RockerOfDpaFlowKey',
> +  'data' : { 'priority': 'uint32', 'tbl-id': 'uint32', '*in-pport': 'uint32',
> +             '*tunnel-id': 'uint32', '*vlan-id': 'uint16',
> +             '*eth-type': 'uint16', '*eth-src': 'str', '*eth-dst': 'str',
> +             '*ip-proto': 'uint8', '*ip-tos': 'uint8', '*ip-dst': 'str' } }

Missing '#optional' tags on the various optional fields.  Why are
certain fields optional?  Does it mean they have a default value, or
that they don't make sense in some configurations?  The docs could be
more clear on that.

> +
> +##
> +# @RockerOfDpaFlowMask:
> +#
> +# Rocker switch OF-DPA flow mask
> +#
> +# @in-pport: physical input port
> +#
> +# @tunnel-id: tunnel ID
> +#
> +# @vlan-id: VLAN ID
> +#
> +# @eth-src: Ethernet header source MAC address
> +#
> +# @eth-dst: Ethernet header destination MAC address
> +#
> +# @ip-proto: IP Header protocol field
> +#
> +# @ip-tos: IP header TOS field
> +##

Missing a 'Since: 2.3' designation.

> +{ 'type': 'RockerOfDpaFlowMask',
> +  'data' : { '*in-pport': 'uint32', '*tunnel-id': 'uint32',
> +             '*vlan-id': 'uint16', '*eth-src': 'str', '*eth-dst': 'str',
> +             '*ip-proto': 'uint8', '*ip-tos': 'uint8' } }

Again, missing #optional tags in the docs, as well as what it means when
a field is omitted.

> +
> +##
> +# @RockerOfDpaFlowAction:
> +#
> +# Rocker switch OF-DPA flow action
> +#
> +# @goto-tbl: next table ID
> +#
> +# @group-id: group ID
> +#
> +# @tunnel-lport: tunnel logical port ID
> +#
> +# @vlan-id: VLAN ID
> +#
> +# @new-vlan-id: new VLAN ID
> +#
> +# @out-pport: physical output port
> +##

Missing a 'Since: 2.3' designation.

> +{ 'type': 'RockerOfDpaFlowAction',
> +  'data' : { '*goto-tbl': 'uint32', '*group-id': 'uint32',
> +             '*tunnel-lport': 'uint32', '*vlan-id': 'uint16',
> +             '*new-vlan-id': 'uint16', '*out-pport': 'uint32' } }

Repeat of #optional comments...

> +
> +##
> +# @RockerOfDpaFlow:
> +#
> +# Rocker switch OF-DPA flow
> +#
> +# @cookie: flow unique cookie ID
> +#
> +# @hits: count of matches (hits) on flow
> +#
> +# @key: flow key
> +#
> +# @mask: flow mask
> +#
> +# @action: flow action
> +#
> +# Since: 2.3
> +##
> +{ 'type': 'RockerOfDpaFlow',
> +  'data': { 'cookie': 'uint64', 'hits': 'uint64', 'key': 'RockerOfDpaFlowKey',
> +            'mask': 'RockerOfDpaFlowMask', 'action': 'RockerOfDpaFlowAction' } }
> +
> +##
> +# @rocker-of-dpa-flows:
> +#
> +# Return rocker OF-DPA flow information.
> +#
> +# @name: switch name
> +#
> +# @tbl-id: (optional) flow table ID.  If tbl-id is not specified, returns

s/(optional)/#optional/ for consistency with other .json files

> +# flow information for all tables.
> +#
> +# Returns: @Rocker OF-DPA flow information
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'rocker-of-dpa-flows',

Should this command be in the query-* namespace?

> +  'data': { 'name': 'str', '*tbl-id': 'uint32' },
> +  'returns': ['RockerOfDpaFlow'] }
> +
> +##
> +# @RockerOfDpaGroup:
> +#
> +# Rocker switch OF-DPA group
> +#
> +# @id: group unique ID
> +#
> +# @type: group type
> +#
> +# @vlan-id: VLAN ID
> +#
> +# @pport: physical port number
> +#
> +# @index: group index, unique with group type
> +#
> +# @out-pport: output physical port number
> +#
> +# @group-id: next group ID
> +#
> +# @set-vlan-id: VLAN ID to set
> +#
> +# @pop-vlan: pop VLAN headr from packet
> +#
> +# @group-ids: list of next group IDs
> +#
> +# @set-eth-src: set source MAC address in Ethernet header
> +#
> +# @set-eth-dst: set destination MAC address in Ethernet header
> +#
> +# @ttl-check: perform TTL check
> +##

Missing a 'Since: 2.3' designation.

> +{ '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' } }

Again on #optional...

> +
> +##
> +# @rocker-of-dpa-groups:
> +#
> +# Return rocker OF-DPA group information.
> +#
> +# @name: switch name
> +#
> +# @type: (optional) group type.  If type is not specified, returns
> +# group information for all group types.
> +#
> +# Returns: @Rocker OF-DPA group information
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'rocker-of-dpa-groups',
> +  'data': { 'name': 'str', '*type': 'uint8' },
> +  'returns': ['RockerOfDpaGroup'] }
> +
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 8957201..9007c12 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3902,3 +3902,100 @@ Move mouse pointer to absolute coordinates (20000, 400).
>  <- { "return": {} }
>  
>  EQMP
> +
> +    {
> +        .name       = "query-rocker",

Doesn't match the naming in your .json file (but I do like query-rocker
better).

> +        .args_type  = "name:s",
> +        .mhandler.cmd_new = qmp_marshal_input_rocker,
> +    },
> +
> +SQMP
> +Show rocker switch
> +------------------
> +
> +Arguments:
> +
> +- "name": switch name
> +
> +Example:
> +
> +-> { "execute": "query-rocker", "arguments": { "name": "sw1" } }
> +<- { "return": {"name": "sw1", "ports": 2, "id": 1327446905938}}

Is there a command to learn which switch names exist?  As written, this
command assumes you already know the switch name, which makes the switch
name on output a bit redundant.  My suggestion above was to make 'name'
be optional on input, and return an array on output.

> +
> +EQMP
> +
> +    {
> +        .name       = "query-rocker-ports",

Doesn't match .json naming.

> +        .args_type  = "name:s",
> +        .mhandler.cmd_new = qmp_marshal_input_rocker_ports,
> +    },
> +
> +SQMP
> +Show rocker switch ports
> +------------------------
> +
> +Arguments:
> +
> +- "name": switch name
> +
> +Example:
> +
> +-> { "execute": "query-rocker-ports", "arguments": { "name": "sw1" } }
> +<- { "return": [ {"duplex": "full", "enabled": true, "name": "sw1.1", "autoneg": "off", "link-up": true, "speed": 10000},
> +                 {"duplex": "full", "enabled": true, "name": "sw1.2", "autoneg": "off", "link-up": true, "speed": 10000}
> +   ]}
> +
> +EQMP
> +
> +    {
> +        .name       = "query-rocker-of-dpa-flows",

Another naming mismatch.

> +        .args_type  = "name:s,tbl-id:i?",
> +        .mhandler.cmd_new = qmp_marshal_input_rocker_of_dpa_flows,
> +    },
> +
> +SQMP
> +Show rocker switch OF-DPA flow tables
> +-------------------------------------
> +
> +Arguments:
> +
> +- "name": switch name
> +- "tbl-id": (optional) flow table ID
> +
> +Example:
> +
> +-> { "execute": "query-rocker-of-dpa-flows", "arguments": { "name": "sw1" } }
> +<- { "return": [ {"key": {"in-pport": 0, "priority": 1, "tbl-id": 0},
> +                  "hits": 138,
> +                  "cookie": 0,
> +                  "action": {"goto-tbl": 10},
> +                  "mask": {"in-pport": 4294901760}
> +                 },
> +                 {...more...},
> +   ]}
> +
> +EQMP
> +
> +    {
> +        .name       = "query-rocker-of-dpa-groups",

and again

> +        .args_type  = "name:s,type:i?",
> +        .mhandler.cmd_new = qmp_marshal_input_rocker_of_dpa_groups,
> +    },
> +
> +SQMP
> +Show rocker OF-DPA group tables
> +-------------------------------
> +
> +Arguments:
> +
> +- "name": switch name
> +- "type": (optional) group type
> +
> +Example:
> +
> +-> { "execute": "query-rocker-of-dpa-groups", "arguments": { "name": "sw1" } }
> +<- { "return": [ {"type": 0, "out-pport": 2, "pport": 2, "vlan-id": 3841, "pop-vlan": 1, "id": 251723778},
> +                 {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3841, "pop-vlan": 1, "id": 251723776},
> +                 {"type": 0, "out-pport": 1, "pport": 1, "vlan-id": 3840, "pop-vlan": 1, "id": 251658241},
> +                 {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840, "pop-vlan": 1, "id": 251658240}
> +   ]}
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]