"dev" <dev-boun...@openvswitch.org> wrote on 06/16/2016 10:58:58 PM:
> From: Yusheng Wang <yshw...@vmware.com> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: "dev@openvswitch.org" <dev@openvswitch.org> > Date: 06/16/2016 10:59 PM > Subject: Re: [ovs-dev] [PATCH] OVN: initial patch of datalog engine > Sent by: "dev" <dev-boun...@openvswitch.org> > > > I agree with all your comments but the last one. The benefit of > using one source file is > that you know everything is there as far as the engine is concerned. > We definitely need > to separate it once it is really large. > > I suppose it will take some time for the code to stabilize and the > engine will evolve over > time. The package right now is self contained and people could try > with it using the > interactive test case -- writing a datalog program, presenting it > with some input and > seeing what comes out. The previous man page have the implementation details > and it is a good start point before diving into the code. > > As for the prefix, I was thinking about 'dl' but it might be too > short to distinguish itself > while 'datalog' might be too long considering all functions will > carry it. Since test cases > are living in another source file, quite a few internal functions > have to be declared as > external so a lot more functions have to carry that name. > > I noticed the automake conflict when I sync my repo. I am not sure > if we have the guideline > of not changing the last line which does not have back slash char, > instead adding new lines > in the middle. Hope this will not prevent people from patching and > trying with it. The conflict is caused by a later change to ovn/lib/automake.mk. A quick fix is to change line 43 to line 45 in your patch. - Hui > > -- Yusheng > ________________________________________ > From: Ryan Moats <rmo...@us.ibm.com> > Sent: Friday, June 17, 2016 12:03 AM > To: Yusheng Wang > Cc: dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH] OVN: initial patch of datalog engine > > This commit message is too bare for a commit this size. > Please provide some content for later readers that aren't > up to speed with the technology being introduced here. > > Because the patch is so big, I'm not including my comments > in-line, I'm going to put them all here: > > I know there was a separate patch for the ovn-datalog man patch, > I think that patch should be folded into this one for a more > atomic approach. > > Note: the log_ prefix is already used in lib/util.h and while this > patch introduces a private include file, I'd be more comfortable > if the log_ prefix were replaced with _datalog_ to be more consistent > with the ovs naming convention. > > Similarly, this patch overloads other prefixes that are used > elsewhere - I'd consider cleaning up the prefix usage to > avoid confusion later on. > > Nit: is there any reason why ENT_* values 0x0c through 0x0f > aren't in numerical order like the entries from 0 to 0x0b > appear to be? > > Nit (found in multiple places): comments should begin with a > capital letter and end with a period > > Also, I'm thinking that the datalog source file should > be broken into multiple files rather than concatenated together > (as it is over 3K lines long) > > Lastly, this patch set has a collision in the automake.mk file > with the current master and so could use a rebase. > > Ryan Moats > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
