Hi Cathy, Thanks for the review. Below is my response to your comments. Please let me know if you have any additional questions.
-- Quaker ===================================================== REVIEWER: Cathy.Zhou at Sun.COM WEBREV: http://cr.grommit.com/~zf162725/cr_0308/ FILES: GLDv3 Userland NOTES: Description of feedbacks: ACCEPT Request accepted REJECT Request rejected EXPLAIN Explanation given DISCUSS Request requires further discussion to resolve DEFER Request deferred (e.g. because work is out-of-scope) ===================================================== usr/src/cmd/dladm/dladm.c usr/src/lib/libdladm/common/libdladm.h usr/src/lib/libdladm/common/secobj.c usr/src/uts/common/io/dld/dld_drv.c usr/src/uts/common/sys/dld.h usr/src/lib/libwladm/Makefile.com No Comments usr/src/lib/libwladm/common/libwladm.c (not including the smf changes) 1. I don't like the fact that do_connect() and do_disconnect() takes two arguments fd and link which refers to basically the same object. I understand that the fundamental problem behind this is libwladm trying to reuse fd for every operation, but which I think it is unnecessary. There are some other problems and inconsistencies - we don't need to reuse gbuf in every functioins libwladm calls. Another example is some of do_xxx() function returns wladm_status_t and some of them returns int. So ideally, for example, I prefer the do_get_capability() function to be in the form of: wladm_status_t do_get_capability(const char *link, wl_capability_t *cap); I understand some of these problems already existed before your changes, but as you are the same project team working on the libwladm code before, I hope you can make the necessary changes so that this project and the future ones won't add some strange code to work around these problems. | DEFER | As you said, these problems exist before this project, so I suggest we open a bug to track | this issue, and fix in another build. 2. do_connect() Why not call wpa_instance_delete() when connecting attempt fails or timeout on L602 and L608? | EXPLAIN | When dladm disconnect-wifi, it will call do_disconnect(), and do_disconnect() will call wpa_instance_delete(). | When do_connect() fails, it will call do_disconnect(), so the wpa_instance_delete() will be executed. 3. wladm_ioctl_set() and wladm_ioctl_get() I don't think it is right to introduce these two generic libwladm APIs (Do they PSARCed?) I think it makes more sense to have library functions like: wladm_set_wpa(), wladm_del_key() etc. usr/src/lib/libwladm/common/libwladm.h usr/src/lib/libwladm/common/mapfile-vers The same as 3. | ACCEPT | Remove the wladm_ioctl_set() and wladm_ioctl_get(), replaced with high level APIs, | such as wladm_set_wpa(), wladm_del_key(), and so on. | When the code review complete, we will return to PSARC to record these APIs. Besides, the change webrev is at: http://cr.grommit.com/~zf162725/cr_0322/
