Looks good, thanks Jan. /jb On Aug 18, 2011, at 5:35 PM, Jan Damborsky wrote:
> Thank you for review, Jesse. > > Please see my response in-line. > > Jan > > > On 08/18/11 17:51, Jesse Butler wrote: >> >> This looks good. My only high level comment is that my assumption around >> "label" and "name" are the opposite of what you have - I feel like the >> actual NIC driver name and instance (e.g. 'e1000g2') would be referred to as >> "name", and the vanity name (e.g. 'net3') might be considered a 'label'. >> I'll leave it up to you whether you want to pursue that or not... > > Based on your and Dave's comment, I went with 'name:device' pair instead of > 'name:label'. > >> >> One minor comment, as well. In network_info.py, on line 161 is it necessary >> to check both len returned and also for each element to be set? In other >> words, is it possible to get a return back that doesn't include the NIC >> driver+instance (e.g. 'e1000g2'). If not, you could just check for len == 2 >> and assign, right? >> >> if len(vanity_device) == 2: >> (n_name, n_label) = vanity_device > > In general, "dladm -p" can return ":value" or "value:" - e.g. > > # dladm show-link -o link,over -p > e1000g0: > e1000g1: > vboxnet0: > net0:e1000g1 > net1:e1000g1 > # > > So the question is if that could happen in our particular case - > for instance if vanity name is not available for given link while network > device is. In such case dladm would return something like ':bge0'. > > To be honest, I haven't seen that during my testing, but I am not > sure we can say for sure it can't happen now or in future either > as a intentional change in dladm or as a bug. > So, I think we should be defensive and try to cope with that situation. > > Also, during further testing I found out that in some cases > (pre-vanity system updated to post-vanity one), vanity name equals > to device name: > > # dladm show-phys -L -o vanity,device -p > e1000g0:e1000g0 > e1000g1:e1000g1 > # > > I have modified the code, so that in this case, only 'NIC name' > part of dictionary is populated and device name portion is left > with empty string, as I believe displaying the same value twice > may be redundant. > > I have updated webrev accordingly: > > full: > https://cr.opensolaris.org/action/browse/caiman/dambi/cr-7078155-cr/webrev-cr/ > > incremental: > https://cr.opensolaris.org/action/browse/caiman/dambi/cr-7078155-cr-diff/webrev-cr-diff/ > >> >> Best >> Jesse >> >> >> On Aug 18, 2011, at 10:29 AM, Jan Damborsky wrote: >> >>> Hi all, >>> >>> could I please ask two pair of eyes to take a look at fix for >>> >>> 7078155 snv171 text installer provides insufficient NIC information after >>> vanity-naming-by default went in. >>> >>> webrev: >>> https://cr.opensolaris.org/action/browse/caiman/dambi/cr-7078155/webrev/ >>> >>> Thank you, >>> Jan >>> >>> tests done: >>> >>> * installation with modified Sparc text installer booted from network >>> * installation with modified x86 text installer booted from media >>> * configuration of non-global zone with exclusive IP stack >>> >>> _______________________________________________ >>> caiman-discuss mailing list >>> [email protected] >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> > _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
