Thank you Seb and sorry for the delay in responding as I was on vacation yesterday.
Sebastien Roy wrote: >> http://cr.grommit.com/~sshakya/dlpi_notification/ [external] > > This is a good addition to this library. Here are my comments: > > libdlpi.c > > 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. > > 760: I don't think this does what you intend. This check will succeed > if some of the notification types requested are unsupported but if at > least one is supported. > I'll fix this. > 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. > > 771: Is this resolved? No. I haven't tested this out. but will updated once I do test it. > > 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. Note if i were to address your comment for line 1697 I think I will have to revise this approach. > > 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. > > 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 */ > > 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? > > 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. > > 1706: This isn't a walking function. A walking function does > something for every node in a data structure. This function simply > seeks to tell the caller whether a given node exists. I'd call it > i_dlpi_notifyidexists(), and have it return boolean_t. > I've made the changes. > > libdlpi.h > > 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"? > > 150: This belongs in the union since it only applies to > DL_NOTE_PHYS_ADDR. I'd place physaddrlen and physaddr in a structure > within that union. > I'll change this. > > libdlpi_impl.h > > 69: This is the hard-coded list of notifications that consumers of > this API are allowed to request. This list and the contents of > dlpi_notifyinfo_t must always be in sync with the list of DLPI > notifications in dlpi(7P). There's no way to guarantee that these > will be in sync, but adding a comment to that effect couldn't hurt. I'll add a comment here explaining the case. Thanks, Sagun
