Hi Samuel,
> <PATCH>
> >All NET_BUFFERs of a NET_BUFFER_LIST must go through the pipeline:
> >extract, find flow, execute. Previously, only the first NET_BUFFER of a
> >NET_BUFFER_LIST was going through this pipeline, which was erroneous.
> </PATCH>
> comments here.
>
> It might make the reading clearer, and strengthen my confidence that I did
> not miss something :)
>
Sorry about that. My client displays it nicely so I didn't realize it was
getting difficult to read. I see your point though. I find [QUOTE] [/QUOTE]
mechanism a little difficult to follow with nested comments. I will continue
using my current approach, which Ben also recommended, of adding an additional
'>' character to previous comments. In addition, I will also trim down the
content that is not relevant. Hopefully this will make it easier to see my
comments. Let me know if this still doesn't help.
> Nit: the parameter should be on a new line.
> [/QUOTE]
> I think we would also need a coding style update here: any function
> declaration or definition should have each parameter on a separate line.
> I am going to fix the modif for this function you pointed.
Hm, I was under the impression that this was already a part of the style.
Thanks for fixing it anyways.
> [QUOTE]
> This function has become a bit too long now, can you please refactor it?
> One obvious thing you could do is to extract the processing of an single NB
> into a separate function and the parent function can take care of splitting
> the
> NBL (if required) & creating the right forwarding ctx.
> [/QUOTE]
> Perhaps I should refactor OvsStartNBLIngress in a separate commit.
> I would not like to combine multiple issues in a single commit. Plus, it would
> look clearer in the history.
I strongly prefer/advice not to do such refactoring in a separate review. It
wasn't fun reviewing this function at all. I spent way more time reviewing it &
at the end I was still not confident about my review. That is why I asked for a
refactor. Doing the refactoring now has a lot more context than doing it at a
later point. Not to mention this unnecssarily adds more burden to the review
effort. We spend good amount of time reviewing a long function call only to
review it again when the same logic gets refactor. I see that you have already
sent out a separate review for refactoring, so I will take a look. But I really
really don't prefer this.
> Yes, I think it would be a good idea to have only one style of multi-line
> comments.
> Even though, I personally prefer this style:
> /*
> * comment line
> * comment line
> */
>
I take back my earlier comment. While I still prefer the more condense form of
commenting, I think most of our code does follow the style that your prefer.
Presumably because others prefer that style too. So let's keep that going. We
should clarify this in the style guide.
Thanks,
Saurabh
PS: Hopefully this reply is easier to follow.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
