Cathy Zhou wrote: > Cathy Zhou wrote: >> Dan Groves wrote: >>> Hi, >>> >>> I posted a new link ID management API document at: >>> >>> http://www.opensolaris.org/os/project/clearview/uv/link_id_management.pdf >>> >>> >> It looks good. Some initial comments at the first glance: >> >> 1. This type enumerates the different link classes supported. The >> enumeration consists of: DLADM_CLASS_PHYS ... >> >> Please be clear the listed classes are only the ones supported for >> now, we'd expect other new classes be added, for example, >> DLADM_CLASS_IPTUN. >> >> 2. dladm_walk_linkids >> >> dladm_status_t dladm_walk_link(void (*fn)(const char *, void *), >> void *arg, dladm class t class, uint t flags) >> >> I would think that the signature of the function be: >> >> dladm_status_t dladm_walk_link(void (*fn)(dladm_linkid_t, void *), >> void *arg, dladm class t class, uint t flags) >> >> 3. How to get the link name, link class from one specific dladm_conf_t? >> > some more comments follow: > > 4. The signature of get/set_conf_field should be: > > dladm_status_t dladm_get_conf_field(dladm_conf_t conf, const char* field, > void *value, size_t *vallenp); > > dladm_status_t dladm_set_conf_field(dladm_conf_t conf, const char *field, > void char *value, size_t vallen); >
I can go either way on this. I remember meem made the comment that we should pass strings. I don't have a strong feeling any more for either way of doing things. > 5. It is either dladm_walk_link() or dladm_walk_linkids(). Please fix > one of them. > It's supposed to be dladm_walk_linkids. I've fixed it. > 6. the return type of the callback function called by dladm_walk_link() > is better to return integer instead of void. That is either > DLADM_WALK_CONTINUE or DLADM_WALK_TERMINATE > Done. I also made the same change for dladm_walk_conf. > 7. dladm_status_t dladm_create_linkid(const char *, dladm_class_t, > uint_t, dladm_linkid_t *); > Oops... I fixed that. > 8. I would think dladm_walk_conf() will only be called in devfsadmd, so > it should be made private (not in this document). > Right now only devfsadmd uses dladm_walk_conf, but could someone in the future want to use it? I'd prefer to leave it as is. > 9. Note the altroot argument in below functions: > > dladm_status_t dladm_write_conf(dladm_conf_t conf, const char *altroot); > dladm_status_t dladm_remove_conf(dladm_linkid_t linkid, > const char *altroot); > Meem mentioned dladm_set_rootdir, which I think I would be better than having extra arguments. thanks, Dan
