Hui Kang/Watson/IBM wrote on 06/17/2016 11:04:21 AM:
> From: Hui Kang/Watson/IBM > To: Yusheng Wang <yshw...@vmware.com> > Cc: Ryan Moats/Omaha/IBM@IBMUS, "dev@openvswitch.org" <dev@openvswitch.org> > Date: 06/17/2016 11:04 AM > Subject: Re: [ovs-dev] [PATCH] OVN: initial patch of datalog engine > > "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. Sorry, this fix is to the automake.mk in a previous patch for the datalog man page. > > - 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
