[ovs-dev] [PATCH 01/23] db-ctl-base: Don't die in parse_column_names() on error.

Jakub Sitnicki jkbs at redhat.com
Tue Jul 3 08:41:27 UTC 2018


Hi Mark,

Thanks for starting the review.

On Mon, 2 Jul 2018 10:55:01 -0400
Mark Michelson <mmichels at redhat.com> wrote:

> Your cover letter still hasn't come through, so feel free to tell me if 
> my comments here are misguided.

Looks like the cover letter has arrived now.
 
> I'm guessing that you did not remove ctl_fatal() calls from 
> parse_commands() and ctl_parse_commands() because there is no 
> ctl_context present. However, it's not clear why the ctl_fatal() call in 
> cmd_add() was not converted to use ctl_error() + return.

I left ctl_parse_commands() & parse_commands() as is for now because
I'm not sure yet if I will end up using them for the ovn-nbctl daemon
mode in their current form.

cmd_add(), OTOH, is an oversight. Thanks for pointing it out. Let me
correct that in v2.
 
> I think patches 15 and 19 could be combined into one.
> 
> You *could* combine patches 1-13 into one patch, 16-18 into one patch, 
> and 20-23 in one patch, but I don't feel strongly enough about this to 
> make you have to re-do the whole thing.

Squashing patches together is not a problem. It depends on what what we
agree on to be one logical change.

I find smaller patches easier to review without losing the context
midway through it. Not to the point when the patch appears disjointed,
though.

Thanks,
Jakub


More information about the dev mailing list