[ovs-dev] [ofp-print 18/18] ofp-print, ofp-parse: Add support for NXAST_REG_MOVE and NXAST_REG_LOAD.
Ben Pfaff
blp at nicira.com
Thu Dec 9 19:02:04 UTC 2010
On Thu, Dec 09, 2010 at 12:27:56AM -0800, Justin Pettit wrote:
> On Dec 8, 2010, at 4:27 PM, Ben Pfaff wrote:
>
> > +static const char *
> > +parse_nxm_field_bits(const char *s, uint32_t *headerp, int *ofsp, int *n_bitsp)
> > +{
> > ...
> > + if (sscanf(s, "[%d..%d]", &start, &end) != 2) {
> > + if (sscanf(s, "[%d]", &start) != 1) {
> > + ovs_fatal(0, "%s: syntax error expecting [<bit>] or "
> > + "[<start>..<end>]", full_s);
> > + } else {
> > + end = start;
> > + }
>
> It surprises me that "src[start]" would be accepted to mean copy a
> single bit, but just specifying "src" to mean the entire field isn't
> supported. I'm not sure how useful the former is, and the latter
> seems quite handy.
It was a little more work to find the end of the field name if there
wasn't always going to be a [ there, so I didn't do it. I hoped no one
would notice.
I've added this now, but since I was still feeling lazy I decided to
require the brackets anyway, e.g. src[].
> > +static void
> > +format_nxm_field_bits(struct ds *s, uint32_t header, int ofs, int n_bits)
> > +{
> > + format_nxm_field_name(s, header);
> > + if (n_bits != 1) {
> > + ds_put_format(s, "[%d..%d]", ofs, ofs + n_bits - 1);
> > + } else {
> > + ds_put_format(s, "[%d]", ofs);
> > + }
>
> Do you think it's worth sanity-checking that "n_bits" is not 0? It
> would be possible with a malformed "move" action.
It ought to be clear enough to the reader when he sees [5..4] or
whatever.
> > +.IP "\fBmove:\fIsrc\fB[\fIstart\fB:\fIend\fB]->\fIdst\fB[\fIstart\fB:\fIend\fB]\fR"
> > +Copies the named bits from field \fIsrc\fR to field \fIdst\fR.
> > +\fIsrc\fI and \fIdst\fR must be NXM field names as defined in
> > +\fBnicira\-ext.h\fR, e.g. \fBNXM_OF_UDP_SRC\fR or \fBNXM_NX_REG0\fR.
> > +Each \fIstart\fR and \fIend\fR pair, which are inclusive, must specify
> > +the same number of bits and must fit within its respective field.
> > +Example: \fBmove:NXM_NX_REG0[0..5]\->NXM_NX_REG1[26..31]\fR copies the
> > +six bits numbered 0 through 5, inclusive, in register 0 into bits 26
> > +through 31, inclusive.
> > +.IP "\fBload:\fIvalue\fB\->\fIdst\fB[\fIstart\fB:\fIend\fB]"
> > +Writes \fIvalue\fR to bits \fIstart\fR through \fIend\fR, inclusive,
> > +in field \fBdst\fR. Example: \fBload:55\->NXM_NX_REG2[0..5]\fR loads
> > +value 55 (bit pattern \fB110111\fR) into bits 0 through 5, inclusive,
> > +in register 2.
>
> Was that MIME-encoded? Yeesh. :-)
AES-encrypted :-)
> Do you mind putting a "." line between the move and load definitions?
Done.
> In the third line, that should be an "\fR" after "src". In both
> definitions, the describe "[start:end]", but it should be
> "[start..end]".
Thanks, fixed. (I wanted to use : before I realized that it was
preempted for separating actions from their arguments).)
Thanks for the review.
More information about the dev
mailing list