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: [EMAIL PROTECTED]
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/
_______________________________________________
networking-discuss mailing list
[email protected]
