Thank you very much for review, Jesse.
Jan
On 08/19/11 11:53, Jesse Butler wrote:
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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
