[ovs-dev] [PATCH] OVN: initial patch of datalog engine

[Hui Kang]

Hui Kang/Watson/IBM wrote on 06/17/2016 11:04:21 AM:

> From: Hui Kang/Watson/IBM
> To: Yusheng Wang <yshwang at vmware.com>
> Cc: Ryan Moats/Omaha/IBM at IBMUS, "dev at openvswitch.org"
<dev at openvswitch.org>
> Date: 06/17/2016 11:04 AM
> Subject: Re: [ovs-dev] [PATCH] OVN: initial patch of datalog engine
>
> "dev" <dev-bounces at openvswitch.org> wrote on 06/16/2016 10:58:58 PM:
>
> > From: Yusheng Wang <yshwang at vmware.com>
> > To: Ryan Moats/Omaha/IBM at IBMUS
> > Cc: "dev at openvswitch.org" <dev at openvswitch.org>
> > Date: 06/16/2016 10:59 PM
> > Subject: Re: [ovs-dev] [PATCH] OVN: initial patch of datalog engine
> > Sent by: "dev" <dev-bounces at 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 <rmoats at us.ibm.com>
> > Sent: Friday, June 17, 2016 12:03 AM
> > To: Yusheng Wang
> > Cc: dev at 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 at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev