Yeah, I think this looks like the right direction. I probably wouldn't
have bothered writing a to_python for ofp_action_header since we know
it's not used anywhere else, and would have just handled it in the one
for Flow_stats itself, but this is not really here nor there.
This is a fine use for reinterpret_cast (except for the last one which
you probably noticed needs a little work ;) ).
I don't think that the length of the actions are checked anywhere (I
could be wrong on this), so it might be nice to make sure that the
length field of an action actually matches the size of the struct you're
casting it to. Really we should handle this gracefully (e.g., log it
and don't Pythonize it), but personally, I'd be okay with just doing an
assert, since I would not expect to ever actually see it, but an assert
is better than a segfault if it does pop up. :) Of course, there's also
the argument that we should be validating them earlier. Whatever. Up
to you what (if anything) you want to do about it in any case.
-- Murphy
On Fri, 2011-01-28 at 16:50 +0900, Derek Cormier wrote:
> Without minding the typos, that is...
>
> On 01/28/2011 04:46 PM, Derek Cormier wrote:
> > Ok I'll give it a try and submit a patch when I'm finished. Could you
> > please tell me if the following is
> > the right approach?
> >
> > template <>
> > PyObject*
> > to_python(const ofp_action_header &a)
> > {
> > PyObject* dict = PyDict_New();
> > if (!dict) {
> > return 0;
> > }
> >
> > uint16_t type = ntohs(a.type);
> > pyglue_setdict_string(dict, "type", to_python(type));
> > pyglue_setdict_string(dict, "length", to_python(ntohs(a.len));
> >
> > /* depending on the action type, cast to the appropriate
> > * action struct to get its fields. */
> > if (type == OFPAT_OUTPUT) {
> > const ofp_action_output& ao =
> > reinterpret_cast<ofp_action_output&>(a);
> > uint16_t port = ntohs(ao.port);
> >
> > pyglue_setdict_string(dict, "port", to_python(port));
> >
> > /* max_len only has meaning when the destination port is the
> > controller */
> > if (port == OFPP_CONTROLLER) {
> > pyglue_setdict_string(dict, "max_len",
> > to_python(ntohs(ao.max_len)));
> > }
> > }
> > else if (type == OFPAT_STRIP_VLAN) {
> > /* nothing to set, no struct beyond the header */
> > }
> > else if (type == OFPAT_SET_VLAN_VID) {
> > const ofp_action_vlan_vid av =
> > reinterpret_cast<ofp_action_output&>(a);
> > ....
> >
> > Is this the proper use of reinterpret cast? I've never had to use it
> > before...
> >
> > -Derek
> >
> > On 01/28/2011 03:20 PM, James "Murphy" McCauley wrote:
> >> I believe the issue is that with the to_python() for ofp_flow_stats&, we
> >> have no idea if the actions actually follow the ofp_flow_stats structure
> >> since, for example, someone could have just made an ofp_flow_stats
> >> struct and tried to pythonize it. I am not sure if this ever actually
> >> happens, but whatever. It's not provably a safe thing to do. However,
> >> the Flow_stats struct actually explicitly has the actions wrapped up
> >> into a vector, so it IS possible to safely pull them out of that. It
> >> just hasn't been done.
> >>
> >> The to_python() for Flow_stats should: Step one, convert the fields from
> >> the ofp_flow_stats struct itself into "dict", which is done by just
> >> calling the to_python() for ofp_flow_stats. Step two, unpack v_actions
> >> from the Flow_stats struct into a list of dicts and throw it into "dict"
> >> too. Except instead of step two, we have /* XXX actions */. :) You
> >> should be able to just go ahead and implement it there.
> >>
> >> -- Murphy
> >>
> >> On Fri, 2011-01-28 at 14:50 +0900, Derek Cormier wrote:
> >>> Hello,
> >>>
> >>> I was looking at pyglue.cc and it looks like actions are never included
> >>> in python events.
> >>>
> >>> A comment says to use Flow_stats, but flow stats contains a vector
> >>> ofp_action_header's. Since actions are variable-length, won't this cut
> >>> off data when the action is longer than the header? (For example,
> >>> ofp_action_dl_addr).
> >>>
> >>> So, is there any way to get actions in events like flow stats in? If
> >>> not, how could we go about implementing this?
> >>>
> >>> Thanks,
> >>> Derek
> >>>
> >>> _______________________________________________
> >>> nox-dev mailing list
> >>> nox-dev@noxrepo.org
> >>> http://noxrepo.org/mailman/listinfo/nox-dev_noxrepo.org
> >>
> >>
> >
>
_______________________________________________
nox-dev mailing list
nox-dev@noxrepo.org
http://noxrepo.org/mailman/listinfo/nox-dev_noxrepo.org
