Hi Sowmini, Sowmini.Varadhan at Sun.COM wrote: > dladm.c: >> * 4517: What are the error semantics of DLADM_STATUS_BUSY? It looks >> like if DLADM_STATUS_BUSY is returned, we silently don't do anything >> and pretend that the command succeeded. > linkprop.c >> * 595: If you got DLADM_STATUS_BUSY at 584, the caller will never know, >> and you might return DLADM_STATU_OK, thus pretending that the property >> was successfully set when it wasn't. Honestly, I don't understand why >> DLADM_STATUS_BUSY isn't a regular error like any other error. > > Accept (both of above are related). > > Regarding the semantics of DLADM_STATUS_BUSY: > The intention is that if the driver cannot set the property because it > is busy, then it will return the DLADM_STATUS_BUSY error code to > the caller, but the setting is still stored in the dladm persistent > repository (to be attempted at the next driver attach, via the > daemon). The processing of DLADM_STATUS_BUSY is more integral to the > persistence component, so it will be treated as any other error for the > framework.
I still don't understand what this will look like from the perspective of the person typing "dladm". Will an error now get printed stating that the property wasn't set? If so, how does the administrator know that the property will somehow get set later via some other mechanism, and how do they know when that will happen? Why wouldn't failing the entire operation be better? > And, while fixing this, I noticed that show-ether was missing in > dladm.xcl. I have added this. Okay. >> usr/src/lib/libdladm/common/linkprop.c >> >> * 218-241: Why are these needed? Are there actual alignment issues, or >> are these used to quiet lint? Could memcpy() not be used instead? >> * 223: Why htonl()? Since we're dealing with values that don't show up >> on the wire, intuitively, we should be dealing with values in >> host-byte-order. Am I missing something? >> * 1927: I don't see the need for GETINT64() here. Why not a simple >> memcpy()? > > These were used to quiet lint errors- I was reluctant to suppress > the lint message by other means, since the layout of the structure > can change in the future, and we would have to deal with the alignment > problem at that time anyway. The htonl itself was used > to deal with byte-ordering differences (memcpy will run into > byte-ordering differences, and doing the byte-swap is messy for > int64's). I'll comment this to make it clearer. What properties are stored or returned in network byte-order? >> * 584: The semantics should be that if DLADM_OPT_ACTIVE isn't set, then >> the caller doesn't want the changes applied to the running kernel. >> While dladm currently doesn't have a command-line flag to do >> persistent-only operations, it doesn't seem too hard to implement that >> in the API. > > need clarification: this would also be something that applies > to the parent clearview-uv gate, right? i.e., your comment is that > i_dladm_set_linkprop should only be invoked from dladm_set_linkprop > if DLADM_OPT_ACTIVE is set, correct? Not quite; My comment as it relates to this specific piece of code is that i_dladm_set_linkprop() should not be called if DLADM_OPT_ACTIVE isn't set. The higher-level issue with the entire libdladm API is that the active configuration should be modified if and only if DLADM_OPT_ACTIVE is set, and the persistent configuration should be modified if and only if DLADM_OPT_PERSIST is set. The API is inconsistent, as there are some cases where the active configuration is unconditionally modified regardless of the flags (as is the case here). You're right that the UV gate has this problem; and this inconsistency wasn't introduced by Brussels. Feel free to defer this to Clearview and I'll see if we can fix this in the UV gate. > linkprop.c: >> * 1606: There is no need to map the linkid to a link name. Your ioctl >> just maps the link name back to a linkid in the kernel. It would be >> much simpler to just pass down the linkid. As you have things now, >> there are three mappings: >> >> 1. dladm is passed a link name and maps that to a linkid to call >> dladm_set_linkprop(). >> 2. dladm_set_linkprop() ends up in dld_buf_init_common(), which maps >> that back to a link name so that it can call your DLD ioctl. >> 3. The ioctl maps that back to a linkid so that it can look up the >> link in the dls hash table. >> >> Only (1) is needed. > > also relates to: > >> usr/src/uts/common/io/dls/dls_vlan.c >> >> * 967,982: The model for the dls client APIs is that they take a >> datalink_id_t to identify which datalink to operate on (the >> i_dls_vlan_hash is by datalink_id_t.) Passing in a link name means >> that the dls module has to map this to a datalink_id_t before doing >> anything, which means a round trip up to user-space. If you follow >> the trail in the code you have, which does a dls_vlan_hold_by_name(), >> eventually, you end up in dls_mgmt_get_linkid(), which does the upcall >> to the dlmgmtd daemon. >> >> Having these functions take a linkid would allow them to do a simple >> dls_vlan_hold() instead of dls_vlan_hold_by_name(). You'd then do the >> link-name to datalink_id_t translation in user-space prior to calling >> the libdladm API. >> >> Such a change would ripple up to the ioctl handling in dld, obviously. > > Defer: this will imply some changes to the ARC'ed interfaces, and > is a little complex to define, without the supporting UV defintions. > We'll do this part as part of the Property Persistence component > of Brussels. I'm not sure I see the relationship with persistence. Do you mean that you'd rather not change the interfaces because they're currently defined in a way that is independent of the putback order of UV/Brussels? >> * 1711: I fundamentally disagree with the idea that libdladm somehow >> knows something about acceptable link MTU values. Each driver must >> check to make sure that the given MTU is acceptable anyway, so there's >> no need for libdladm to do anything. Besides, references to Ethernet >> and IP are out of place, as you might be dealing with a datalink that >> has nothing to do with either. > > Both of the above comments are related to the same question: should > libdladm do preliminary bound checks in user-space, or should > we rely on the driver to do all checks. One of the requirements > raised in early Brussels design was to try to do bound checking in > user-space as far as possible. > > I do agree that checking the mtu itself can't be done reliably > for all drivers (as the comments in dld_defmtu_check indicate). So > I'll remove the ip-biased mtu check from defmtu_check. Okay. >> * 3000: The mac_info_t must be comprised of immutable values (see the >> comment in mac.h.) The idea is that mac_info_t is copiedThe min and >> max sdu need to be moved out of mac_info_t, and directly into >> mac_impl_t. > > This is actually something that I imported from clearview (which I just > noticed has been updated). I'm not sure what you mean. The mac_info_t in the clearview gate has not contained the min and max sdu since the MAC-level MTU interfaces have existed in there. > BTW, we had discussed this earlier, and I > believe we concurred that we'd need a fast-track for this interface- > can the clearview-iptun help me with that part by fast-tracking this > into Nevada? Which interfaces are you proposing be deferred? > When I tried updating as you suggest, > I found this change impacts several files that access mi_sdu_max > such as xnbo.c etc. so I'd like to defer this update for cv-iptun. If Brussels is proposing to have a dynamic MTU property, then my preference would be that the stack needs to be able to handle it with the Brussels putback. That said, I don't think I fully understand which interfaces you propose be left in Brussels, and which be deferred until Clearview iptun. Can you clarify? Thanks, -Seb
