[quagga-dev 12682] Re: v1 - Patchwork & ack't patches

Greg Troxel gdt at ir.bbn.com
Tue Jun 9 23:58:57 BST 2015


Vincent JARDIN <vincent.jardin at 6wind.com> writes:

> I did try to browse status on patchwork (I do not want to debate if
> patchwork or not patchwork ;) ) in order to drain the list,
>
> The following are ack't:
>   http://patchwork.quagga.net/patch/1257/

That seems fine.  Although I would be tempted to add a #warning for the
case of inlined system .h, as it seems wrong.

>   http://patchwork.quagga.net/patch/409/
>   http://patchwork.quagga.net/patch/406/
>   http://patchwork.quagga.net/patch/408/
>   http://patchwork.quagga.net/patch/411/
>   http://patchwork.quagga.net/patch/407/

Having read the comments at 407, I'm not at all comfortable that I
understand what's going on.  It's not clear (from the missing commit
messages in these patches :-) where the matching lock acquisitions are,
why it's ok to use the variables after the unlocks, why info is null if
unlock is called, etc.  I know I've been saying things like this on the
list for years, but I would like to see a commit series that adds enough
comments around the locking rules so that one can review the code and
have some confidence that it's correct.  (I don't have confidence now
except that I am not seeing vast numbers of bug reports.)

So I don't think these are ready, but if you feel that you really
understand what's going on, I won't object.

> can be taken:
>   http://patchwork.quagga.net/patch/1249/

It strikes me that in v4, 'sho ip route' is system wide, and then there
are per-protocol commands, so 'sho ip mroute' should in theory be about
all mcast routes that zebra has, separate from where they come from.  So
this seems backwards.

>   http://patchwork.quagga.net/patch/1246/

looks good to me

>   http://patchwork.quagga.net/patch/1245/

looks good to me (assuming it has been tested by someone)

>   http://patchwork.quagga.net/patch/1240/

This looks like it is gratuitously dropping a lot of const, and I don't
see the point.  Maybe that's because I didn't read the missing commit
message.

> minor update to be sent, then could be merged:
>   http://patchwork.quagga.net/patch/1248/ -> Please, Donald?

looks ok to me.

> nack:
>   http://patchwork.quagga.net/patch/410/

Delete away - that is a long-resolved discussion.

>   http://patchwork.quagga.net/patch/537/

This still seems to contain a hint about something wrong, but that seems
like it belongs in bugzilla not patchwork.  I am ambivalent about
deleting it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 180 bytes
Desc: not available
URL: <http://lists.quagga.net/pipermail/quagga-dev/attachments/20150609/700aa4b5/attachment-0001.sig>


More information about the Quagga-dev mailing list