Re: [ovs-dev] [PATCH 6/6] ofp-print: Move much of the printing code into message-specific files.
On Tue, Mar 13, 2018 at 04:15:08PM -0700, Justin Pettit wrote: > > > > On Mar 13, 2018, at 4:04 PM, Justin Pettit wrote: > > > > > >> On Feb 16, 2018, at 2:54 PM, Ben Pfaff wrote: > > > > It looks like this was mostly moving code around, so I didn't pore over the > > review, but let me know if you want me to take a closer look. I did notice > > a few smaller things: > > > >> diff --git a/lib/ofp-table.c b/lib/ofp-table.c > >> index 558e4bcd9127..7df7167deddb 100644 > >> --- a/lib/ofp-table.c > >> +++ b/lib/ofp-table.c > >> ... > >> +const char * > >> +ofputil_table_eviction_to_string(enum ofputil_table_eviction eviction) > >> +{ > >> +switch (eviction) { > >> +case OFPUTIL_TABLE_EVICTION_DEFAULT: return "default"; > >> +case OFPUTIL_TABLE_EVICTION_ON: return "on"; > >> +case OFPUTIL_TABLE_EVICTION_OFF: return "off"; > >> +default: return "***error***"; > >> +} > >> + > >> +} > > > > This seems to have an unnecessary blank line. > > > >> +const char * > >> +ofputil_table_vacancy_to_string(enum ofputil_table_vacancy vacancy) > >> +{ > >> +switch (vacancy) { > >> +case OFPUTIL_TABLE_VACANCY_DEFAULT: return "default"; > >> +case OFPUTIL_TABLE_VACANCY_ON: return "on"; > >> +case OFPUTIL_TABLE_VACANCY_OFF: return "off"; > >> +default: return "***error***"; > >> +} > >> + > >> +} > > > > Same here. > > > >> +void > >> +ofputil_table_desc_format(struct ds *s, const struct ofputil_table_desc > >> *td, > >> + const struct ofputil_table_map *table_map) > >> +{ > >> ... > >> +} > >> + > >> + > >> /* This function parses Vacancy property, and decodes the > > > > And here. > > > >> /* Convert 'setting' (as described for the "mod-table" command > >> * in ovs-ofctl man page) into 'tm->table_vacancy->vacancy_up' and > >> * 'tm->table_vacancy->vacancy_down' threshold values. > >> * For the two threshold values, value of vacancy_up is always greater > >> * than value of vacancy_down. > >> * > >> - * Returns NULL if successful, otherwise a malloc()'d string describing > >> the > >> + * Returns NULL if successful, otherwise a malloc()'d s describing the > > > > I assume truncating "string" to "s" was unintentional. > > > > Thanks for tackling "ofp-print.c". > > > > --Justin > > Oh, and: > > Acked-by: Justin Pettit > > --Justin > > Thanks. I fixed up all of that and I'll apply this series to master in a few minutes. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 6/6] ofp-print: Move much of the printing code into message-specific files.
> On Mar 13, 2018, at 4:04 PM, Justin Pettit wrote: > > >> On Feb 16, 2018, at 2:54 PM, Ben Pfaff wrote: > > It looks like this was mostly moving code around, so I didn't pore over the > review, but let me know if you want me to take a closer look. I did notice a > few smaller things: > >> diff --git a/lib/ofp-table.c b/lib/ofp-table.c >> index 558e4bcd9127..7df7167deddb 100644 >> --- a/lib/ofp-table.c >> +++ b/lib/ofp-table.c >> ... >> +const char * >> +ofputil_table_eviction_to_string(enum ofputil_table_eviction eviction) >> +{ >> +switch (eviction) { >> +case OFPUTIL_TABLE_EVICTION_DEFAULT: return "default"; >> +case OFPUTIL_TABLE_EVICTION_ON: return "on"; >> +case OFPUTIL_TABLE_EVICTION_OFF: return "off"; >> +default: return "***error***"; >> +} >> + >> +} > > This seems to have an unnecessary blank line. > >> +const char * >> +ofputil_table_vacancy_to_string(enum ofputil_table_vacancy vacancy) >> +{ >> +switch (vacancy) { >> +case OFPUTIL_TABLE_VACANCY_DEFAULT: return "default"; >> +case OFPUTIL_TABLE_VACANCY_ON: return "on"; >> +case OFPUTIL_TABLE_VACANCY_OFF: return "off"; >> +default: return "***error***"; >> +} >> + >> +} > > Same here. > >> +void >> +ofputil_table_desc_format(struct ds *s, const struct ofputil_table_desc *td, >> + const struct ofputil_table_map *table_map) >> +{ >> ... >> +} >> + >> + >> /* This function parses Vacancy property, and decodes the > > And here. > >> /* Convert 'setting' (as described for the "mod-table" command >> * in ovs-ofctl man page) into 'tm->table_vacancy->vacancy_up' and >> * 'tm->table_vacancy->vacancy_down' threshold values. >> * For the two threshold values, value of vacancy_up is always greater >> * than value of vacancy_down. >> * >> - * Returns NULL if successful, otherwise a malloc()'d string describing the >> + * Returns NULL if successful, otherwise a malloc()'d s describing the > > I assume truncating "string" to "s" was unintentional. > > Thanks for tackling "ofp-print.c". > > --Justin Oh, and: Acked-by: Justin Pettit --Justin ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 6/6] ofp-print: Move much of the printing code into message-specific files.
> On Feb 16, 2018, at 2:54 PM, Ben Pfaff wrote: It looks like this was mostly moving code around, so I didn't pore over the review, but let me know if you want me to take a closer look. I did notice a few smaller things: > diff --git a/lib/ofp-table.c b/lib/ofp-table.c > index 558e4bcd9127..7df7167deddb 100644 > --- a/lib/ofp-table.c > +++ b/lib/ofp-table.c > ... > +const char * > +ofputil_table_eviction_to_string(enum ofputil_table_eviction eviction) > +{ > +switch (eviction) { > +case OFPUTIL_TABLE_EVICTION_DEFAULT: return "default"; > +case OFPUTIL_TABLE_EVICTION_ON: return "on"; > +case OFPUTIL_TABLE_EVICTION_OFF: return "off"; > +default: return "***error***"; > +} > + > +} This seems to have an unnecessary blank line. > +const char * > +ofputil_table_vacancy_to_string(enum ofputil_table_vacancy vacancy) > +{ > +switch (vacancy) { > +case OFPUTIL_TABLE_VACANCY_DEFAULT: return "default"; > +case OFPUTIL_TABLE_VACANCY_ON: return "on"; > +case OFPUTIL_TABLE_VACANCY_OFF: return "off"; > +default: return "***error***"; > +} > + > +} Same here. > +void > +ofputil_table_desc_format(struct ds *s, const struct ofputil_table_desc *td, > + const struct ofputil_table_map *table_map) > +{ > ... > +} > + > + > /* This function parses Vacancy property, and decodes the And here. > /* Convert 'setting' (as described for the "mod-table" command > * in ovs-ofctl man page) into 'tm->table_vacancy->vacancy_up' and > * 'tm->table_vacancy->vacancy_down' threshold values. > * For the two threshold values, value of vacancy_up is always greater > * than value of vacancy_down. > * > - * Returns NULL if successful, otherwise a malloc()'d string describing the > + * Returns NULL if successful, otherwise a malloc()'d s describing the I assume truncating "string" to "s" was unintentional. Thanks for tackling "ofp-print.c". --Justin ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev