crossbow discuss - Jan 2007 - Code review changes to IP Instances

Hello, David,

Thanks for your careful review!

Unless otherwise noted we''ve applied your suggested changes.

And please find responses and commentaries below.

usr/src/cmd/zoneadm/zoneadm.c
>  Line 2923 - This initialization can be moved to where "self" is
>  declared two lines above.  That said, it''s not clear if
"self"
>  really serves any purpose.  Isn''t it sufficient to just
"break"
>  at each point where you are setting "self" to B_TRUE?
The initialization has been moved as suggested. but "self" check
is necessary, since we have to output the warning in the if block.

usr/src/cmd/zoneadmd/vplat.c
>  Lines 2444 and 2453 - Are these actually warnings or things a
>  user is likely to encounter due to misconfiguration?  The
>  warnings on lines 2436 (interface in use by the global zone)
>  and 2464 (unsupported interface type) seem to be true warnings
>  but the others seem to reflect a system issue, no?  If they''re
>  not supposed to happen, I would remove the "WARNING: " tag
>  (just as they are in remove_datalink().)
They are warnings, actually these warning messages tell user that
something is wrong, but anyway, fail of a network interfaces shouldn''t
hinder the zone from booting.

On the other hand, failure in remove_datalink is an error, it means
that the zone can''t be shut down cleanly.

usr/src/lib/libzonecfg/common/libzonecfg.c
>  Line 1508 - Is Z_BAD_PROPERTY the right value to be checking
>  here?  I guess I would have thought it was the case where
>  getrootattr() returned Z_OK but property[0] == ''\0''
(which you
>  essentially do a few lines down.)  I guess I''m trying to
>  understand in which cases you''ll see this error code and why is
>  it all right to return "shared" in this case?
Z_BAD_PROPERTY is returned in fetchprop (called by getrootattr):
        if ((property = xmlGetProp(cur, propname)) == NULL)
                return (Z_BAD_PROPERTY);
This means there''s no ip-type in the xml file, in which case
the ip-type should be shared. that could be:
1. It''s a zone created previously when there''s no ip-type,
we''ll
    take it as shared.
2. It''s a shared IP zone. (we don''t write "shared",
as you have noticed.)

Thanks again for your review.

Best,

Donghai.

Dong-Hai,
> usr/src/cmd/zoneadm/zoneadm.c
>>  Line 2923 - This initialization can be moved to where "self"
is
>>  declared two lines above.  That said, it''s not clear if
"self"
>>  really serves any purpose.  Isn''t it sufficient to just
"break"
>>  at each point where you are setting "self" to B_TRUE?
> The initialization has been moved as suggested. but "self" check
> is necessary, since we have to output the warning in the if block.
That''s true but after setting self to B_TRUE on line 2931, you only
check for it being B_FALSE until you finally break on line 2968.  I
think this can be written along the lines of

 				/*
 				 * If the zone being verified is
 				 * running and owns the interface
 				 */
 				target_zid = getzoneidbyname(target_zone);
 				if (target_zid == dl_owner_zid)
 					break;

 				/* Zone id match failed, use name to check */
 				if (getzonenamebyid(dl_owner_zid,
 				    dl_owner_zname, ZONENAME_MAX) < 0) {
 					/* No name, show ID instead */
 					(void) snprintf(...
 				} else if (strcmp(dl_owner_zname,
 				    target_zone) == 0)
 					break;

 				/*
 				 * Note here we only report a warning that
 				 .
 				 .
 				(void) fprintf(stderr,
> usr/src/cmd/zoneadmd/vplat.c
>>  Lines 2444 and 2453 - Are these actually warnings or things a
>>  user is likely to encounter due to misconfiguration?  The
>>  warnings on lines 2436 (interface in use by the global zone)
>>  and 2464 (unsupported interface type) seem to be true warnings
>>  but the others seem to reflect a system issue, no?  If
they''re
>>  not supposed to happen, I would remove the "WARNING: " tag
>>  (just as they are in remove_datalink().)
> They are warnings, actually these warning messages tell user that
> something is wrong, but anyway, fail of a network interfaces
shouldn''t
> hinder the zone from booting.
But are these messages emitted because of a misconfiguration on the
part of the administrator?  I think one way the message on line 2444
would be printed if one changes the configuration of an already running
zone using zonecfg(1M) to use an interface already assigned to another
running zone.  Then, the first zone is then rebooted from within zone
itself.  In that case, zoneadm(1M) would not be invoked and the above
code in zoneadm.c would not be executed.

Instead, zoneadmd itself would tear down and then try to bring up the
(revised) virtual platform.  In that particular case, you would see the

 	WARNING: unable to add network interface bge5

message so the warning makes sense.  What would be *even better* is if
similar code to the above checking in zoneadm.c be added here so
instead you get the more precise

WARNING: skipping network interface ''bge5'' which is used by
the non-global zone: twilight

We''ve made a conscious attempt to keep zoneadm and zoneadmd in sync
with this respect, so that errors you might encounter with "zoneadm
verify" will also be emitted by zoneadmd when the zone is rebooted
(from within the zone itself.)  It''s made for some duplicate messages
between the two programs which should eventually be factored out into a
common library.
> On the other hand, failure in remove_datalink is an error, it means
> that the zone can''t be shut down cleanly.
What about failures of dladm_hold_link()?  Are these typically due to
misconfiguration (I can see that if the link name specified is >IFNAMSIZ but
in what other situation?)

Also, I just noticed that dladm_hold_link() doesn''t really return a
boolean so please compare it != 0 on line 2450 of vplat.c.
> usr/src/lib/libzonecfg/common/libzonecfg.c
>>  Line 1508 - Is Z_BAD_PROPERTY the right value to be checking
>>  here?  I guess I would have thought it was the case where
>>  getrootattr() returned Z_OK but property[0] == ''\0''
(which you
>>  essentially do a few lines down.)  I guess I''m trying to
>>  understand in which cases you''ll see this error code and why
is
>>  it all right to return "shared" in this case?
> Z_BAD_PROPERTY is returned in fetchprop (called by getrootattr):
>        if ((property = xmlGetProp(cur, propname)) == NULL)
>                return (Z_BAD_PROPERTY);
> This means there''s no ip-type in the xml file, in which case
> the ip-type should be shared. that could be:
> 1. It''s a zone created previously when there''s no
ip-type, we''ll
>    take it as shared.
> 2. It''s a shared IP zone. (we don''t write
"shared", as you have noticed.)
Okay, thanks for the explanation.

dsc

Hello, David,
> > usr/src/cmd/zoneadm/zoneadm.c
> >>  Line 2923 - This initialization can be moved to where
"self" is
> >>  declared two lines above.  That said, it''s not clear if
"self"
> >>  really serves any purpose.  Isn''t it sufficient to just
"break"
> >>  at each point where you are setting "self" to B_TRUE?
> > The initialization has been moved as suggested. but "self"
check
> > is necessary, since we have to output the warning in the if block.
> 
> That''s true but after setting self to B_TRUE on line 2931, you
only
> check for it being B_FALSE until you finally break on line 2968.  I
> think this can be written along the lines of ...
OK, I''ll remove "self", thanks.
> > usr/src/cmd/zoneadmd/vplat.c
> >>  Lines 2444 and 2453 - Are these actually warnings or things a
> >>  user is likely to encounter due to misconfiguration?  The
> >>  warnings on lines 2436 (interface in use by the global zone)
> >>  and 2464 (unsupported interface type) seem to be true warnings
> >>  but the others seem to reflect a system issue, no?  If
they''re
> >>  not supposed to happen, I would remove the "WARNING: "
tag
> >>  (just as they are in remove_datalink().)
> > They are warnings, actually these warning messages tell user that
> > something is wrong, but anyway, fail of a network interfaces 
> shouldn''t> hinder the zone from booting.
> 
> But are these messages emitted because of a misconfiguration on the
> part of the administrator?  I think one way the message on line 2444
> would be printed if one changes the configuration of an already 
> runningzone using zonecfg(1M) to use an interface already assigned 
> to another
> running zone.  Then, the first zone is then rebooted from within zone
> itself.  In that case, zoneadm(1M) would not be invoked and the above
> code in zoneadm.c would not be executed.
> 
> Instead, zoneadmd itself would tear down and then try to bring up the
> (revised) virtual platform.  In that particular case, you would see 
> the
> 	WARNING: unable to add network interface bge5
> 
> message so the warning makes sense.  What would be *even better* is if
> similar code to the above checking in zoneadm.c be added here so
> instead you get the more precise
> 
> WARNING: skipping network interface ''bge5'' which is used
by the non-
> global zone: twilight
That''s a good idea, I''ll add it.
> We''ve made a conscious attempt to keep zoneadm and zoneadmd in
sync
> with this respect, so that errors you might encounter with "zoneadm
> verify" will also be emitted by zoneadmd when the zone is rebooted
> (from within the zone itself.)  It''s made for some duplicate
messages
> between the two programs which should eventually be factored out 
> into a common library.
I''ll double check the code to see if we break this.
Thanks.
> > On the other hand, failure in remove_datalink is an error, it means
> > that the zone can''t be shut down cleanly.
> 
> What about failures of dladm_hold_link()?  Are these typically due to
> misconfiguration (I can see that if the link name specified is >>
IFNAMSIZ but in what other situation?)
Yes, and it fails when open DLD_CONTROL_DEV fails, or dls_valn_hold fails, 
which would be rare, but anyway, we need it for logical completeness.
Also, it''s hard to tell user what''s going on (such as opening
DLD_CONTROL_DEV
failed...), so just "unable to hold..."
> Also, I just noticed that dladm_hold_link() doesn''t really return
a
> boolean so please compare it != 0 on line 2450 of vplat.c.
OK

Best,

Donghai.

>> Instead, zoneadmd itself would tear down and then try to bring up the
>> (revised) virtual platform.  In that particular case, you would see
>> the
>> 	WARNING: unable to add network interface bge5
>>
>> message so the warning makes sense.  What would be *even better* is if
>> similar code to the above checking in zoneadm.c be added here so
>> instead you get the more precise
>>
>> WARNING: skipping network interface ''bge5'' which is
used by the non-
>> global zone: twilight
> That''s a good idea, I''ll add it.
Thanks for looking into this change; let me know when you''re ready for
me to rereview that file.
>> We''ve made a conscious attempt to keep zoneadm and zoneadmd in
sync
>> with this respect, so that errors you might encounter with
"zoneadm
>> verify" will also be emitted by zoneadmd when the zone is rebooted
>> (from within the zone itself.)  It''s made for some duplicate
messages
>> between the two programs which should eventually be factored out
>> into a common library.
> I''ll double check the code to see if we break this.
> Thanks.
dsc