[ovs-dev] [v12 06/11] dpif-netdev: Add packet count and core id paramters for study

[Van Haaren, Harry]

Hi All,

Eelco, thanks for the reviews. We've addressed your concerns as much as possible, there is one bool (mfex_name_is_study) remaining as
it was required for simplification of logic, and avoiding 3x strcmp() calls on mfex_name.

A v13 will be posted to mailing list shortly. Request to focus only on functional issues (if any).
Assuming there are no functional issues detected in v13, it is time to merge.

Regards, -Harry


From: Eelco Chaudron <echaudro at redhat.com>
Sent: Thursday, July 15, 2021 11:46 AM
To: Van Haaren, Harry <harry.van.haaren at intel.com>
Cc: Amber, Kumar <kumar.amber at intel.com>; ovs-dev at openvswitch.org; fbl at sysclose.org; i.maximets at ovn.org; Ferriter, Cian <cian.ferriter at intel.com>; Stokes, Ian <ian.stokes at intel.com>
Subject: Re: [v12 06/11] dpif-netdev: Add packet count and core id paramters for study


On 15 Jul 2021, at 12:31, Eelco Chaudron wrote:

On 15 Jul 2021, at 12:10, Van Haaren, Harry wrote:

-----Original Message-----
From: Eelco Chaudron <echaudro at redhat.com<mailto:echaudro at redhat.com>>
Sent: Thursday, July 15, 2021 11:08 AM
To: Van Haaren, Harry <harry.van.haaren at intel.com<mailto:harry.van.haaren at intel.com>>
Cc: Amber, Kumar <kumar.amber at intel.com<mailto:kumar.amber at intel.com>>; ovs-dev at openvswitch.org<mailto:ovs-dev at openvswitch.org>;
fbl at sysclose.org<mailto:fbl at sysclose.org>; i.maximets at ovn.org<mailto:i.maximets at ovn.org>; Ferriter, Cian <cian.ferriter at intel.com<mailto:cian.ferriter at intel.com>>;
Stokes, Ian <ian.stokes at intel.com<mailto:ian.stokes at intel.com>>
Subject: Re: [v12 06/11] dpif-netdev: Add packet count and core id paramters for
study

<snip>

Aha, yes OK I see what you're suggesting clearly now.

Personally I liked the "only process extra args if study" trick (by "indenting" it a

level),

but I'll rework to your suggestion here for simplicity/consistency.

Thanks looking forward to v13, a nice number to sign off on ;)

Hah yeah. To keep you interested, below the diff to fixup the new behaviour.
Note that study_count must be initialized to the default value, as it is an optional
argument and there is no "} else {" clause to handle the default value anymore.

v13 on its way, -Harry

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 14ccca7a03..476678e9fa 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1088,7 +1088,7 @@ dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc,
const char *mfex_name = NULL;
const char *reply_str = NULL;
struct ds reply = DS_EMPTY_INITIALIZER;
- unsigned int study_count = 0;
+ unsigned int study_count = MFEX_MAX_PKT_COUNT;
int err;
struct shash_node *node;

@@ -1122,23 +1122,17 @@ dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc,
argc -= 1;
argv += 1;

- /* If name is study and more args, parse study_count value. */
- if (strcmp("study", mfex_name) == 0) {
- if (argc >= 2) {
- if (!str_to_uint(argv[1], 10, &study_count) ||
- (study_count == 0)) {
- ds_put_format(&reply,
- "Error: Invalid study_pkt_cnt value: %s.\n",
- argv[1]);
- goto error;
- }
-
- argc -= 1;
- argv += 1;
- } else {
- study_count = MFEX_MAX_PKT_COUNT;
- }

Now I remember what I was thinking ;)

You do the compare here:

   if (strcmp("study", mfex_name))

       study_count = MFEX_MAX_PKT_COUNT;

This way you can use !study_count in the rest of the code.

+ /* If name is study and more args exist, parse study_count value. */
+ } else if (mfex_name && strcmp("study", mfex_name) == 0) {
+ if (!str_to_uint(argv[1], 10, &study_count) ||
+ (study_count == 0)) {
+ ds_put_format(&reply,
+ "Error: Invalid study_pkt_cnt value: %s.\n", argv[1]);
+ goto error;
}
+
+ argc -= 1;
+ argv += 1;
} else {
ds_put_format(&reply, "Error: unknown argument %s.\n", argv[1]);
goto error;

Note that that might mess up the output at the end on successful completion, and setting the value globally.