Re: [zones-discuss] Webrev for CR 6782448
Frank Batschulat (Home) wrote: On Wed, 23 Dec 2009 01:34:59 +0100, Jordan Vaughan wrote: http://cr.opensolaris.org/~flippedb/onnv-zone2 [...] zone_lookup_nwif() needs the three loop checks. I regenerated the webrev. You'll notice that the assertion was replaced by a check that returns Z_INSUFFICIENT_SPEC. Hey Jordan, thanks for the exhaustive reply. understood. I was ignoring the fact that without these checks the xml parsing loop would generate false alarm for such conditions: net: address: 10.5.234.15/24 physical: bge0 defrouter not specified zonecfg:mojo> select net address=10.5.234.15/24 select net: No such resource with that id lgtm! cheers frankB Thanks Frank. Jordan ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] Webrev for CR 6782448
On Wed, 23 Dec 2009 01:34:59 +0100, Jordan Vaughan wrote: >>> http://cr.opensolaris.org/~flippedb/onnv-zone2 [...] > zone_lookup_nwif() needs the three loop checks. > > I regenerated the webrev. You'll notice that the assertion was replaced > by a check that returns Z_INSUFFICIENT_SPEC. Hey Jordan, thanks for the exhaustive reply. understood. I was ignoring the fact that without these checks the xml parsing loop would generate false alarm for such conditions: net: address: 10.5.234.15/24 physical: bge0 defrouter not specified zonecfg:mojo> select net address=10.5.234.15/24 select net: No such resource with that id lgtm! cheers frankB ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] Webrev for CR 6782448
Hi Frank, Thanks for reviewing my fix. I'll respond to your questions below. On 12/22/09 05:55 AM, Frank Batschulat (Home) wrote: On Sat, 19 Dec 2009 04:28:52 +0100, Jordan Vaughan wrote: I expanded my webrev to include my fix for 6910339 zonecfg coredumps with badly formed 'select net defrouter' I need someone to review my changes. The webrev is still accessible via http://cr.opensolaris.org/~flippedb/onnv-zone2 Hey Jordan looks good to me modulo this in zonecfg_lookup_nwif() size_t addrspec;/* nonzero if tabptr has IP addr */ size_t physspec;/* nonzero if tabptr has interface */ +size_t defrouterspec; /* nonzero if tabptr has def. router */ if (tabptr == NULL) return (Z_INVAL); + * zone_nwif_address, zone_nwif_physical, and zone_nwif_defrouter are + * arrays, so no NULL checks are necessary. */ addrspec = strlen(tabptr->zone_nwif_address); physspec = strlen(tabptr->zone_nwif_physical); -assert(addrspec > 0 || physspec > 0); +defrouterspec = strlen(tabptr->zone_nwif_defrouter); +assert(addrspec != 0 || physspec != 0 || defrouterspec != 0); so we do consider any of them being 0 a fault given the assert(), fine, but yet we do check for this again inside the loop: +if (physspec != 0 && (fetchprop(cur, DTD_ATTR_PHYSICAL, +physical, sizeof (physical)) != Z_OK || +strcmp(tabptr->zone_nwif_physical, physical) != 0)) +continue; +if (addrspec != 0 && (fetchprop(cur, DTD_ATTR_ADDRESS, address, +sizeof (address)) != Z_OK || +!zonecfg_same_net_address(tabptr->zone_nwif_address, +address))) +continue; +if (defrouterspec != 0 && (fetchprop(cur, DTD_ATTR_DEFROUTER, +address, sizeof (address)) != Z_OK || +!zonecfg_same_net_address(tabptr->zone_nwif_defrouter, +address))) +continue; a good argument could probably be made to turn this assert into a real check and return Z_INVAL for any of those 3 being 0 and get rid of the checks inside the xml parsing loop ? The assertion doesn't fail if any of the three variables is zero; it fails if all of them are zero. However, your suggestion that we transform the assertion into a real check that returns Z_INVAL or Z_INSUFFICIENT_SPEC is good. I was able to easily produce a core dump on my system even without my fix: ---8<--- root arrakis [16:12:49]# zonecfg -z mojo zonecfg:mojo> select net address="" Assertion failed: addrspec > 0 || physspec > 0, file ../common/libzonecfg.c, line 2170 zsh: IOT instruction (core dumped) cz mojo ---8<--- I verified that changing the assertion into a real check that returns Z_INSUFFICIENT_SPEC eliminates the problem: ---8<--- root tcm3000-01 [16:13:03 1]# cz mojo zonecfg:mojo> select net address="" select net: Insufficient specification ---8<--- However, the three checks in the loop (physspec != 0, etc.) are necessary even after converting the assertion into a non-asserting test. Suppose that a zone were to have the following net configuration: ---8<--- zonecfg:mojo> info net net: address: 10.5.234.15/24 physical: bge0 defrouter not specified ---8<--- If I were to eliminate the three checks in the loop, then if I were to issue a "select net address=10.5.234.15/24", then zonecfg(1M) would claim that the zone doesn't have a network resource with an address of 10.5.234.15/24! This follows from the way the three if statements would work without the three aforementioned checks: physspec would be zero (because the query doesn't specify a physical interface) but the network resource's physical property would be nonempty, which would make the strcmp(3C) invocation in the first if statement return a nonzero value and cause the function to skip the network resource that it would have otherwise selected! Here is some output from zonecfg(1M) while it's using a libzonecfg that lacks the three loop checks: ---8<--- root tcm3000-01 [16:25:12 1]# cz mojo zonecfg:mojo> info zonename: mojo zonepath: /export/mojo brand: solaris10 autoboot: true bootargs: pool: limitpriv: scheduling-class: ip-type: shared hostid: net: address: 10.5.234.15/24 physical: bge0 defrouter not specified zonecfg:mojo> select net address=10.5.234.15/24 select net: No such resource with that id zonecfg:mojo> ---8<--- zone_lookup_nwif() needs the three loop checks. I regenerated the webrev. You'll notice that the assertion was replaced by a check that returns Z_INSUFFICIENT_SPEC. Thanks again for reviewing my fix, Jordan cheers frankB ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] Webrev for CR 6782448
On Tue, 22 Dec 2009 14:55:34 +0100, Frank Batschulat (Home) wrote: > a good argument could probably be made to turn this assert into a real > check and return Z_INVAL for any of those 3 being 0 and get rid of > the checks inside the xml parsing loop ? probably rather Z_INSUFFICIENT_SPEC then Z_INVAL though. ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] Webrev for CR 6782448
On Sat, 19 Dec 2009 04:28:52 +0100, Jordan Vaughan wrote: > I expanded my webrev to include my fix for > > 6910339 zonecfg coredumps with badly formed 'select net defrouter' > > I need someone to review my changes. The webrev is still accessible via > > http://cr.opensolaris.org/~flippedb/onnv-zone2 Hey Jordan looks good to me modulo this in zonecfg_lookup_nwif() size_t addrspec;/* nonzero if tabptr has IP addr */ size_t physspec;/* nonzero if tabptr has interface */ +size_t defrouterspec; /* nonzero if tabptr has def. router */ if (tabptr == NULL) return (Z_INVAL); + * zone_nwif_address, zone_nwif_physical, and zone_nwif_defrouter are + * arrays, so no NULL checks are necessary. */ addrspec = strlen(tabptr->zone_nwif_address); physspec = strlen(tabptr->zone_nwif_physical); -assert(addrspec > 0 || physspec > 0); +defrouterspec = strlen(tabptr->zone_nwif_defrouter); +assert(addrspec != 0 || physspec != 0 || defrouterspec != 0); so we do consider any of them being 0 a fault given the assert(), fine, but yet we do check for this again inside the loop: +if (physspec != 0 && (fetchprop(cur, DTD_ATTR_PHYSICAL, +physical, sizeof (physical)) != Z_OK || +strcmp(tabptr->zone_nwif_physical, physical) != 0)) +continue; +if (addrspec != 0 && (fetchprop(cur, DTD_ATTR_ADDRESS, address, +sizeof (address)) != Z_OK || +!zonecfg_same_net_address(tabptr->zone_nwif_address, +address))) +continue; +if (defrouterspec != 0 && (fetchprop(cur, DTD_ATTR_DEFROUTER, +address, sizeof (address)) != Z_OK || +!zonecfg_same_net_address(tabptr->zone_nwif_defrouter, +address))) +continue; a good argument could probably be made to turn this assert into a real check and return Z_INVAL for any of those 3 being 0 and get rid of the checks inside the xml parsing loop ? cheers frankB ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] Webrev for CR 6782448
lgtm.
ed
On Mon, Dec 21, 2009 at 05:41:29PM -0800, Jordan Vaughan wrote:
> That's a good idea. I updated the webrev.
>
> Thanks,
> Jordan
>
> On 12/21/09 05:08 PM, Steve Lawrence wrote:
> >Minor nit. You could use != POC_STRING, put the Z_NO_ENTRY in the {}, and
> >put the success case after. Not a required change.
> >
> >LGTM.
> >
> >-Steve
> >
> >On Fri, Dec 18, 2009 at 07:28:52PM -0800, Jordan Vaughan wrote:
> >>I expanded my webrev to include my fix for
> >>
> >>6910339 zonecfg coredumps with badly formed 'select net defrouter'
> >>
> >>I need someone to review my changes. The webrev is still accessible via
> >>
> >>http://cr.opensolaris.org/~flippedb/onnv-zone2
> >>
> >>Thanks,
> >>Jordan
> >>___
> >>zones-discuss mailing list
> >>zones-discuss@opensolaris.org
>
> ___
> zones-discuss mailing list
> zones-discuss@opensolaris.org
___
zones-discuss mailing list
zones-discuss@opensolaris.org
Re: [zones-discuss] Webrev for CR 6782448
That's a good idea. I updated the webrev.
Thanks,
Jordan
On 12/21/09 05:08 PM, Steve Lawrence wrote:
Minor nit. You could use != POC_STRING, put the Z_NO_ENTRY in the {}, and
put the success case after. Not a required change.
LGTM.
-Steve
On Fri, Dec 18, 2009 at 07:28:52PM -0800, Jordan Vaughan wrote:
I expanded my webrev to include my fix for
6910339 zonecfg coredumps with badly formed 'select net defrouter'
I need someone to review my changes. The webrev is still accessible via
http://cr.opensolaris.org/~flippedb/onnv-zone2
Thanks,
Jordan
___
zones-discuss mailing list
zones-discuss@opensolaris.org
___
zones-discuss mailing list
zones-discuss@opensolaris.org
Re: [zones-discuss] Webrev for CR 6782448
Minor nit. You could use != POC_STRING, put the Z_NO_ENTRY in the {}, and
put the success case after. Not a required change.
LGTM.
-Steve
On Fri, Dec 18, 2009 at 07:28:52PM -0800, Jordan Vaughan wrote:
> I expanded my webrev to include my fix for
>
> 6910339 zonecfg coredumps with badly formed 'select net defrouter'
>
> I need someone to review my changes. The webrev is still accessible via
>
> http://cr.opensolaris.org/~flippedb/onnv-zone2
>
> Thanks,
> Jordan
> ___
> zones-discuss mailing list
> zones-discuss@opensolaris.org
___
zones-discuss mailing list
zones-discuss@opensolaris.org
Re: [zones-discuss] Webrev for CR 6782448
I expanded my webrev to include my fix for 6910339 zonecfg coredumps with badly formed 'select net defrouter' I need someone to review my changes. The webrev is still accessible via http://cr.opensolaris.org/~flippedb/onnv-zone2 Thanks, Jordan ___ zones-discuss mailing list zones-discuss@opensolaris.org
