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. -- 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
