sagun shakya wrote: >> 754: I don't see the point of checking dli_note_processing here. It >> could be set to B_TRUE the instant after you've checked its value >> here. This is not useful without proper synchronization. Since you're >> comfortable documenting that multiple threads shouldn't mess with a >> single dlpi_handle_t, then there's no need to check the flag here. > This check is to catch dlpi_enabnotify() being called by a callback.
Okay. >> 768: dl_timelimit appears to do nothing. Does the original PSARC case >> for the DL_NOTIFY_* stuff specify what it was intended for? > Here is what the original PSARC case design doc. has: > "dl_timelimit specifies the upper bound on how long time the DLS > provider can wait from > an event occuring until reporting it in a DL_NOTIFY_IND. This wait time > allows the driver to potentially recover tfrom link/NIC down events and > reduces flapping should the > link e.g. switch speed very rapidly. The time limit applies to all > notifications but the DLS provider is free to generate the DL_NOTIFY_IND > with a shorter wait time than requested including always generating > immediate notifications. The wait time does not apply for notifications > sent in response to a DL_NOTIFY_REQ to describe the current state of the > DLS provider. " > > But I don't see this timelimit being used by the DLPI provider/drivers. > I setthe value to zero there as the manpage dlpi(7P) states that it is > reserved for future use and must be set to zero. That's fine, I was mainly curious. >> >> 771: Is this resolved? > No. I haven't tested this out. but will updated once I do test it. Okay. >> >> 814: You have a pointer to the node you're trying to delete. A >> back-pointer in the node would allow you to delete it in-place. You >> wouldn't need to walk the entire list looking for the node you're >> already pointing at in order to delete it. > I had thought out having a double linked list but expecting the list > wouldn't grow to large numbers. I settled with the single linked list. I > chose walking the list when deleting nodes over managing a double linked > list. That's fine. > > Note if i were to address your comment for line 1697 I think I will have > to revise this approach. Not necessarily. You can still address 1697 without implementing a doubly linked list. See below. >> >> 1281-1294: This code comes before the checks for MORECTL and MOREDATA. >> Could a DL_NOTIFY_IND not result in a return value of MORECTL? > A DL_NOTIFY_IND could result in a return value of MORECTL if something > goes wrong. I'll move the checks for MORECTL and MOREDATA before > checking for DL_NOTIFY_IND. Okay. >> >> 1399: This is only true if notifications have been enabled. One can >> still "expect" a DL_NOTIFY_IND message through this API without >> enabling notifications, right? > That's correct. I've reworded the comment to: > > 1398 * This routine succeeds if the message is an expected > request/acknowledged > 1399 * message. However, if DLPI notification has been enabled via > 1400 * dlpi_enabnotify(), DL_NOTIFY_IND messages are handled before > handling > 1401 * expected messages. Otherwise, any other unexpected asynchronous > messages will > 1402 * be discarded. > 1403 */ Sounds good. >> >> 1685: You need a default here to handle potential broken drivers that >> pass up unexpected notifications that the consumer doesn't want. > In the case where unexpected notifications are passed up I think they > should be just ignored as the code stand right now. I may be missing > something? Oh, I see. 1691 will ensure that consumers don't get called back for unexpected notifications. That's fine. >> >> 1697: Instead of walking all notification entries everytime you get a >> notification just in case you need to delete an entry, why not check >> to see if a given entry needs to be deleted after you've called its >> function at 1692? You can check dln_rm at that point since you're >> already looking at that node in the list, and you can remove it in-place. > In order to remove it in-place I would have to go with the approach of > double linked list. The flag setting/unsetting at 1688 and 1695 would > have to be changed. i.e: > move 1688 to 1692 and 1695 to 1693. A compromise would be to set a "delete needed" boolean to B_TRUE at 1693 (outside the if statement) if dlm_rm is true, then only call i_dlpi_deletenotifyid() at 1698 if the "delete needed" boolean is set. >> >> 93: In the code, this error code has nothing to do with the link. >> Implementations of the DL_NOTIFY_* mechanism actually don't tell you >> if it didn't know about the notifications you asked to enable in >> DL_NOTIFY_REQ. They just return DL_NOTIFY_ACK and pretend >> everything's okay. > That's true. Would it be appropriate to say "DLPI notification type > unsupported by libdlpi"? How about, "unsupported DLPI notification type"? Thanks, -Seb
