Code review comment for lp://staging/~cviethen/hipl/pisa-pairing-tng

On Mon, Jan 09, 2012 at 02:55:27AM +0000, Christoph Viethen wrote:
> on Jan 9, 2012, at 12:16 AM, Diego Biurrun wrote:
> >> +static int conf_handle_trigger_bex(struct hip_common *const msg,
> >> + const int action,
> >> + const char *opt[],
> >> + const int optc,
> >> + UNUSED int send_only)
> >
> > Ugh..
>
> Please elaborate - are you referring more to the formatting, or to the
> multitude of consts, or ... ?

The formatting, which suggests your uncrustify hook is not working..

> >> + if (action != ACTION_TRIGGER) {
> >> + return -1;
> >> + }
> >> +
> >> + if (optc > 1) {
> >> + HIP_ERROR("Too many arguments for trigger bex command.\n");
> >> + return -1;
> >> + }
> >
> > Could be merged.
>
> Umm, I'm a little slow right now, maybe. I only want that HIP_ERROR()
> output in one of the two cases.

Of course, you are right ..

> >> + const int uh_err = hip_build_user_hdr(msg, HIP_MSG_TRIGGER_OPP_BEX, 0);
> >> +
> >> + if (uh_err) {
> >> + HIP_ERROR("Failed to build user message header: %s\n", strerror(uh_err));
> >> + return -1;
> >> + }
>
> > The variable indirection with uh_err looks inconsistent to me, you don't
> > do it in the other cases.
>
> I guess I was keen to keep things as const correct as possible. One
> might argue, of course, that over such a small stretch of code,
> nothing bad can happen to uh_err, since one can see with a blink of
> the eye that, past initialization, it's not written to any more.

Uh, that variable *is* used in the HIP_ERROR call - I should not
review when I'm tired...

> Many thanks for your thorough review!

It was rather shallow, thankfully Christof reviewed as well :)

Diego