peter.memishian at sun.com wrote:
>I don't think this is the case. The line in question is:
>
> if ((mp = copyb(mp)) == NULL || (mp->b_cont = copyb(mp)) == NULL) {
> freemsg(mp);
> return (ENOMEM);
> }
>
>... but if the first copyb() fails, then it will return NULL and `mp' will
>be assigned to NULL prior to entering the body of the `if'. We will then
>call `freemsg(NULL)' which is documented to do nothing.
>
>
Agreed.
> > * ipif_up(): After the call to ipif_ndp_up, we want to return if
> > the error is EINPROGRESS, but we fall through.
>
>Yes, this is a bug in my changes (thanks for catching it) -- however ...
>
> > But I am not sure about ipif_ndp_up behavior, it does not seem to
> > return the EINPROGRESS I guess something related to Jim's DAD changes?
>
>... it seems that ipif_ndp_up() (even before DAD, according to Jim) has
>never returned EINPROGRESS. So the bug above is an accidental fix of
>sorts ;-) Still, I will restore original behavior, as I don't want to
>modify that logic without a lot more research, and it's out-of-scope for
>my changes.
>
>
ok.
> > * ip_reprocess_ioctl() The check at L25359 is redundant. ipsq_current_ipif
> > is bound to be NULL here. So we can call ipsq_current_start directly.
> > Right ?
>
>This is an interesting case. Is your assertion that ipsq_current_ipif
>must be NULL because if it's non-NULL, the it must still have the original
>IPSQ value and thus the ipsq_try_enter() in ipif_set_values() will always
>trivially succeed (by reentering itself)? If so, I think I agree, but the
>current code in Nevada is not so bold -- that is, ip_reprocess_ioctl()
>does not ASSERT() that ipsq_current_ipif is NULL prior to setting it to
>ill->ill_ipif -- so it ends up working in both cases. The wording of the
>comment in ip_reprocess_ioctl() also suggests that there are cases where
>ipsq_current_ipif is already set.
>
>For now, I've made ip_reprocess_ioctl() mirror the ipsq_current_start()
>code in ipif_set_values() -- but I could be convinced to be more bold.
>
>
ip_reprocess_ioctl is called only from ipsq_exit() during the unwind from
the ipsq and not directly. ipsq_dq() picks the next message from the ipsq.
ipsq_dq() won't pick up the next message from ipsq_xopq_mphead unless
ipsq_current_ipif is NULL. In the case of ipif_set_values -> ipsq_try_enter
it specifies NEW_OP. Based on all the above you can assert that
ipsq_current_ipif
must be NULL in this case.
>Is there an explicit problem with always treating DL_NOTIFY_IND as a new
>operation? Since IP cannot be sure that driver support DL_NOTIFY, it does
>not wait for the response for the drivers when it sends the DL_NOTIFY_REQ
>probe -- so why would it be appropriate to consider the resulting
>DL_NOTIFY_IND's part of the current operation?
>
>
There may not be any problem today. But consider the following.
We start some operation say a plumb which consists of multiple DLPI message
exchanges between IP and the driver. Some message say DL_BIND triggers a
DL_NOTE_LINK_UP. Now we don't process this message immediately and
queue it as a NEW_OP and continue the DLPI message exchange process.
After the plumb is over, this DL_NOTE_LINK_UP is irrelevant, but we pick it
up now and attempt to set the RUNNING flag, but it should already be set.
>Updated webrev:
>
> http://atlantic.east/ws/meem/physaddr/webrev
>
>Thanks again for your help.
>--
>meem
>
>
Do you need the ill_fastpath_flush() in ill_set_phys_addr_tail() ?
Hasn't bringing down and bringing up the interface achieved
that already ?
Thirumalai
