svn commit: r262196 - head/sys/netpfil/pf

[Ermal Luçi]

On Wed, Feb 19, 2014 at 4:32 PM, Gleb Smirnoff <glebius at freebsd.org> wrote:

>   Martin,
>
> M> > On Tue, Feb 18, 2014 at 10:17:12PM +0000, Martin Matuska wrote:
> M> > M> Author: mm
> M> > M> Date: Tue Feb 18 22:17:12 2014
> M> > M> New Revision: 262196
> M> > M> URL: http://svnweb.freebsd.org/changeset/base/262196
> M> > M>
> M> > M> Log:
> M> > M>   De-virtualize pf_mtag_z [1]
> M> > M>   Process V_pf_overloadqueue in vnet context [2]
> M> > M>
> M> > M>   This fixes two VIMAGE kernel panics and allows to simultaneously
> M> > run host-pf
> M> > M>   and vnet jails. pf inside jails remains broken.
> M> > M>
> M> > M>   PR:                kern/182964
> M> > M>   Submitted by:        glebius at FreeBSD.org [2], myself [1]
> M> > M>   Tested by:        rodrigc at FreeBSD.org, myself
> M> > M>   MFC after:        2 weeks
> M> >
> M> > I've sent your patch to Nikos, who is working on pf+vimage. He
> M> > also accumulates his work on pf+vimage in projects/pf branch,
> M> > planning to do it properly and then merge to head in one go.
> M> > I was waiting for his review. Yes, he is slow with reviews,
> M> > but that's not a reason to commit w/o review.
>
> On Wed, Feb 19, 2014 at 02:01:23PM +0100, Martin Matuska wrote:
> M> I understand your point - if anything is broken (or more broken than
> M> before) I can revert this patch anytime.
> M>
> M> FreeNAS and other folks may fork separate branches and we can wait until
> M> about FreeBSD 12.0 for the patch being reviewed so we can commit it
> around
> M> 14.0 - maybe we have switched to a completely different firewall at that
> M> time and this issue becomes obsolete anyway.
>
> No need for sarcasm and top quoting. Since you already got sharp in
> your reply, let me too.
>
> First of all. I did not submitted you [2], right now I just checked
> my sent mail to ensure that. I submitted you other patch, that later
> was rejected by zec@, and that patch was very unlike [2]. So
> statement in commit message is not true.
>
> Second, these two changes are absolutely unrelated. They shouldn't
> been committed as one patch.
>
> Third. As you already know, there is projects/pf branch, where Nicos
> is getting things right wrt pf+VIMAGE. The patches should first go
> to this branch and tested in it. Committing to head (even a good
> code), you are creating conflicts for Nicos. You are fixing two
> particular problems that hurt you, while Nicos tries to get things
> right in general, for everyones sake. My approach on taskqueue
> context (that was rejected by Marko), was also an attempt to
> create a good and generic way of dealing with the problem. Unfortunately,
> Marko didn't suggest good alternatives.
>
> Anyway this is not a reason to plumb problems in place.
>
> As you may notice yourself the code you added:
>
>         if (IS_DEFAULT_VNET(curvnet))
>                 pf_mtag_z = uma_zcreate("pf mtags", sizeof(struct m_tag) +
>                     sizeof(struct pf_mtag), NULL, NULL, pf_mtag_init, NULL,
>                     UMA_ALIGN_PTR, 0);
>
> Is quite not like the rest of the code of the function. That is because
> in head/ the per-VNET initialization in pf isn't separated from global
> initialization. This is a generic problem, that Nikos is solving in
> projects/pf. Making pf mtag zone in projects/pf would be more clean
> than in head. And of course after your change merge of head to
> projects/pf would fail. You could join Nikos efforts, but instead
> you are just putting obstacles on his way. And mine too, since I
> would do next merge.
>
>
Well go do some work instead of runting around.
You did not listen to me as well when you started doing work on pf.



> --
> Totus tuus, Glebius.
>



-- 
Ermal