Jerry,
Thank you for your comments! See inline:
> dev_netops.c:
>
> You might want to add this assert following line 163, as if
> this is not the case, the code will be doing a VN_RELE at failed.
>
> error = sdev_mknode(ddv, nm, &dv, ...
> if (error != 0) {
> ASSERT(dv == NULL);
> ....
>
Accept.
>
> In devnet_lookup(), would it make more sense to declare private
> as a dls_channel_t and cast it to void where necessary rather
> than the other way around?
>
Accept.
> line 172: blank
> line 174: ths -> this
> line 176: udpated -> updated
>
Accept.
> line 190: if sdev_to_vp() were to return an error, whoops.
> (this assumption appears elsewhere, hmmm). ASSERT(error == 0)?
>
Can we assure that sdev_to_vp() must return 0? I don't feel comfortable to
add an assertion here.
> devnet_filldir_datalink(): same suggestion re private
>
Accept.
> lines 284-305: rather than 'goto done', turn the rebuild
> code into a block?
>
To have a "goto" makes the logic more clear. Having a block might result
ugly indent.
> At the conclusion of the rebuild, SDEV_BUILD is cleared.
> Do we know for certain that the dls_devnet_need_rebuild()
> has now been cleared also? Assert this? Redo the
> rebuild?
>
dls_devnet_need_rebuild() clears itself. We don't have to assert this. If it
is then set, the next devnet_filldir() would rebuild the directory.
> line 332: hold -> held
>
Accept.
>
> sdev_impl.h:
>
> sdev_private: turn this into a union with per-directory flavors
> rather than just leaving this as a void * catch-all?
>
To have a union means that when another directory needs to add its own
private data, it has to change the definition of this structure. I'd rather
having a (void *) so that other directory can use this structure directly.
> SDEV_UPDATE: the name of this symbol is rather generic. make it
> more specific and reverse the sense like perhaps SDEV_ATTR_INVALID
>
Accept.
> 242: added an extra unnecessary line
>
Removed.
Thanks again for your comments!
Can you take a look of bug 6616725? I need your opinion on that.
Thanks
- Cathy
