Re: (KAME-snap 9050) Re: Code nit questions...

gnn_at_freebsd.org
Date: 05/17/05

  • Next message: Dean Strik: "Re: Can't export /usr/ports" 
    Date: Tue, 17 May 2005 11:05:10 -0700
To: JINMEI Tatuya / 神明達哉 <jinmei@isl.rdc.toshiba.co.jp>

    At Tue, 17 May 2005 23:04:11 +0900,
    jinmei wrote:
    >
    > >>>>> On Thu, 12 May 2005 22:49:12 -0400,
    > >>>>> gnn@freebsd.org said:
    >
    > > In a continuing effort to clean up some code nits in the IPv6 code
    > > I'd like to propose the following diffs. There is a comment, starting
    > > with a *) explaining the problem and proposed fix.
    >
    > Thanks for your continuous efforts. Here are some comments.

    No problem, it's my pleasure.

    > > *) Insert proper return value checking.
    >
    > in6_embedscope() should not fail in this context (so we could even
    > panic if it does), but you probably want to be very proactive by
    > eliminating as many (hidden) assumptions as possible. If so, please
    > go ahead with the change, but then I'd rather check the return value
    > of in6_recoverscope() as well.

    Checking the return value of in6_recoverscope() seems fine to me if it
    is also OK by you. If you/kame commit that then I'll try to pick up
    the change when it happens.

    > > *) Make sure that sro is also valid before de-referencing it.
    >
    > Aside from the fact that sro is not a pointer type as Andre pointed
    > out, I'm not even sure whether the code around this point is correct
    > in the first place. Rev. 1.30 of in6_src.c currently reads:

    This proposed change was dropped, it was my mistake.

    > > - if (opts && opts->ip6po_pktinfo &&
    > > + if (ifp && opts && opts->ip6po_pktinfo &&
    > > opts-> ip6po_pktinfo->ipi6_ifindex) {
    > > if (!(ifp->if_flags & IFF_LOOPBACK) &&
    > > ifp->if_index !=
    >
    > This one does not seem to harm, although ifp can probably be never
    > NULL in this context as commented around this part (but again, you
    > probably want to be very proactive, then it's fine).

    OK, that's been committed to FreeBSD-CURRENT.

    > > *) Make sure that rule is valid before dereferencing it.
    >
    > (I don't know much about the ip6_fw implementation, so my comment is
    > not based on any expertise of my own as a KAME developer.)
    >
    > Although the check does not seem to harm per se, it looks to me that
    > rule can never be NULL based on the logic of the big for loop above
    > this point. In fact, the code has an assertion check which detects
    > almost the same erroneous condition:
    >
    > #ifdef DIAGNOSTIC
    > /* Rule 65535 should always be there and should always match */
    > if (!chain)
    > panic("ip6_fw: chain");
    > #endif
    >
    > So, there seem to be some inconsistency about unexpected failure.
    >

    Hmm, I'll have to look at that again.

    > > *) Do not bcopy if the pointer is NULL, whether or not canwait was
    > > set.
    >
    > Hmm...can't we assume malloc() returns a valid (non NULL) pointer if
    > its fourth argument is not M_NOWAIT? If we can (i.e, it's a
    > function's feature), "ip6po_nexthop == NULL && canwait != M_NOWAIT"
    > should indicate a serious bug within malloc() or some other parts of
    > the kernel. So I'm not sure if it's really a good idea to pretend
    > that the bug didn't exist...
    >
    > BTW: if you really want to go with this change, you'll also want to
    > make the same change on ip6po_pktinfo for consistency.

    Sigh, that's a good point that neither the tool checking this, nor I,
    took into account. I will most likely reverse this change and put in
    a comment.

    Thanks,
    George
    _______________________________________________
    freebsd-net@freebsd.org mailing list
    http://lists.freebsd.org/mailman/listinfo/freebsd-net
    To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org"

  • Next message: Dean Strik: "Re: Can't export /usr/ports"

    Relevant Pages