Sowmini,
Thank you for your comments. See inline:
> usr/src/cmd/dladm/dladm.c
> - (nit) add a comment over str2int explaining differences from atoi.
> - (nit) line 365 s/possible/possibly
>
Accept.
> usr/src/lib/libdladm/common/libdladm.h
> - add comments explaining DLADM_OPT_* constants.
>
Accept.
> usr/src/lib/libdladm/common/libdlaggr.c
> - lines 487,496,502: can free portstr as part of destroyconf
Reject. destroyconf frees orig_portstr, not portstr.
> - line 514, 701: check unnecessary: free() will return if pointer is null.
Accept.
> - lines 508-509: unecessary- we're going to go to destroyconf regardless
> of status.
Accept.
> Is an else clause missing here for the success case?
No. in the success case, it should also calls dladm_destroy_conf(conf) and
continue.
> Not sure how we would get to line 520 if DLADM_OPT_PERSIST is set.
> See next comment:
Discuss. I am not sure what do you mean here.
> - i_dladm_aggr_add_rmv() :
> There seem to be some constraints about what flags are possible- these
> should be clarified via comments. For example,
> orig_portstr is only calloc-ed if DLADM_OPT_PERSIST is set;
> Is it possible to have !PERSIST but ACTIVE?
Yes. It is.
> If yes, then we would
> get to line 552 with null orig_portstr, and seg fault at the strlen
> in dladm_set_conf_field().
No. Not that line would only be called if DLADM_OPT_PERSIST is set.
> Similarly, is it not possible to have
> both PERSIST and ACTIVE set at the same time?
Yes.
> If yes, then please
> add some comments clarifying this. Otherwise, how would we get to line
> 520 (see previous comment)
I still do not understand what you mean here.
> - i_dladm_aggr_create_sys() : mac_addr can be const u_char *.
Accept.
> - (pre-existing condition) VALID_PORT_MAC should check for, and fail
> if mac_addr is null.
Accept.
> - dladm_aggr_policy2str: check (and bail) if str is null.
First please note this function is not added by Clearview.
Accept.
> Pass in
> size of str, and use strncat instead of strcat (or alternatively,
> allocate str as needed in dladm_aggr_policy2str() and expect the caller
> to free it)
The common practice of the dladmxxx2str() functions in libdladm is that the
size of the input string is DLADM_STRSIZE, so that I will use that size
directly.
> - dladm_aggr_create: why is key == 0 being accepted now?
When aggregation is created using a vanity name (not by giving a key), then
the key is 0.
> - is there a reason why dladm_aggr_delete does not call
> dladm_phys_down()? In other words, the only step skipped by
> dladm_aggr_delete (but performed by dladm_phys_down)
> is dladm_phys_info()- is this an optimization or an oversight?
dladm_phys_down() is called to "Validate (and purge) a physical link if its
associated hardware has been removed during the system shutdown". As
dladm_phys_down() would be called when create or up an aggregation. So an
removed hardware would be detected then, and not needed when you delete this
aggregation.
> - Question: why does dladm_phys_down delete all the active linkprop
> before deleting a link?
>
dladm_phys_down() means this physical link would not be active on this
system, the same as its active linkprops.
> usr/src/lib/libdladm/common/libdllink.c
> - lines 349-351: how is atomicity ensured across the pair
> of {dladm_read_conf(); dladm_write_conf();} operations?
> My understanding (comments in code would help here) is that
> the read_conf() reads out the conf from the avl tree managed
> by dlmgmtd, and the write_conf() will actually write this to
> a file. So is there a possibility that 2 threads simultaneously
> do read_conf, and modifications, but one thread's modifications
> are lost?
>
When one calls dladm_read_conf(), it read a conf_id of a conf avl tree entry
(management by dlmgmtd), in the conf entry, it would keep a ld_gen value,
which is the generation number of a datalink entry managed by dlmgmtd. When
it again calls dladm_write_conf(), dlmgmtd would compare the generation
number to see whether there is any change of the datalink between this two
operation. If there is, it returns EAGAIN.
I will add some comments regarding this in dlmgmtd.
> usr/src/lib/libdladm/common/libdlwlan.c
> - line 852, 865: why the double cast (uintptr_t itself typedef to
> unsigned int in int_types.h)?
Note that uintptr could also be long.
> - line 1085 s/introudce/introduce
> - line 1110 s/indicate/indicates
>
Accept.
> usr/src/lib/libdladm/common/libdlwlan.h
> - move wlan specific enums (e.g., dladm_wlan_radio_t,
> dladm_wlan_powermode_t,
> dladm_wlan_rates_t, radio_vals, powermode_vals) into libdlwlan.h
> so that they are all in one place.
>
I'll move the type definition to libdlwlan_impl.h, but I'd prefer to leave
radio_vals powermod_vals in linkprop.
> usr/src/lib/libdladm/common/libdlwlan_impl.h
> - val_desc_t is really a general purpose structure that is shared
> with linkprop.c (and possibly other files that will be added
> in the future), and not specific to wlan.; not clear why this is
> not defined in libdladm.h instead?
>
Do you expect other files other than libdladm to use it. If not, I'd like
move it to libdladm_impl.h instead of libdladm.h.
> usr/src/lib/libdladm/common/linkprop.c
> - move wlan specific enums (e.g., dladm_wlan_radio_t,
> dladm_wlan_powermode_t,
> dladm_wlan_rates_t, radio_vals, powermode_vals) into libdlwlan.h
> so that they are all in one place.
See above.
> - rename radio_vals to something like dladm_wlan_radio_vals so that
> it follows the convention used by the other wlan vars.
Accept.
> - the implementation of dladm_init_linkprop is much easier to follow!
> what is the intention of the "arg" (unused) argument to
> i_dladm_init_one_prop?
It is required by dladm_walk_linkprop(). You could specify the second
argument, and that will be carries as the third argument of the callback
func of dladm_walk_linkprop(). In this case, the argument is NULL and is not
used.
> - delete extra line at 249.
Accept.
> - This question was asked in one of the mailing lists earlier, but
> since I can't remember the answer, maybe it needs a detailed comment
> in the code: what do the fields pd_optval, pd_noptval mean? When
> adding new properties, how does one determine if these are neeeded?
> They seem to impact do_check_prop, so commenting that function as well
> would help for future reference.
Accept.
> - move MAX_SUPPORT_RATES to one of the libdlwlan* files
>
I am not sure about this. Read the comments above it, it seems a pure
workaround specific for do_check_rate(), and is not used elsewhere.
Thank you again for all your time spend on this.
Thanks
- Cathy
