On 15 Jul 2021, at 11:24, Van Haaren, Harry wrote:
> Hi Eelco,
>
> Thanks for review. Updates with [hvh] prefix below. In general I've fixed-up
> the suggestions,
> with the exception of 1 that I didn't understand or think actually worked.
ACK, will comment on the one remaining item.
> I'll ask Amber to integrate the changes below into the patchset, next version
> to ML soon.
>
> Regards, -Harry
>
<SNIP>
>
> if ((strcmp(miniflow_funcs[MFEX_IMPL_STUDY].name, mfex_name) == 0)
>
> [hvh] It was previously used inside the .c which declares that array. That
> array isn't available in dpif-netdev.c (nor should it be), so the above is
> the pragmatic solution. Study is an exception case in this code snippet
> anyway as it accepts extra args that the other mfex impls don’t.
ACK, you are right! Guess al we need here is to move to strcmp to avoid partial
matches.
<SNIP>
> Below you break again with the idea of doing every parameter in a separate
> case?
> I think it should be something like (not tested, and assuming you removed
> mfex_name_is_study and mfex_name_parsed):
>
> [hvh] I'm not following the suggestion here, sorry. A parameter after
> mfex_name is only valid if mfex_name == "study". The implementation today
> checks that there is another argument IFF mfex impl == study, and consumes it
> as "study count" in that case. That's exactly we want?
>
> [hvh] study_count = 0; as per suggestion above, so the else(… && study_count)
> will always fail, and never execute? Handling argc is the best way to do what
> we want, I see no issue with it, so leaving as is.
>
My bad should have been !study_count
> } else if ((mfex_name && study_count) {
> ·
>
> o if (argc >= 2) {
>
> *
Reading my explanation again, it’s not clear :)
The command line has the following syntax:
dpif-netdev/miniflow-parser-set [-pmd core] miniflow_implementation_name
[study_pkt_cnt]
The goal of the while(argc < 1) loop was to process one argument at the time.
But your current code does the following:
while(argc < 1) {
if pmd:
process pmd:
else name
process name:
if study_count
process study_count
else
Error
}
As you can see, the process study_count is at the wrong level.
My suggestion was to move it to the same level. Something like this:
while(argc < 1) {
if !strcmp(argv[1], "-pmd") && pmd_thread_to_change != NON_PMD_CORE_ID:
process pmd
else !mfex_name
process name
else if (mfex_name && !study_count):
process study_count
else
Error
}
<SNIP>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
