Hi Jack --
Here is the updated code review. Please let me know if it gets your
blessing.
https://cr.opensolaris.org/action/browse/caiman/ginnie/7087609/webrev/
thanks,
ginnie
On 09/ 7/11 11:59 AM, Jack Schwartz wrote:
On 09/ 7/11 10:24 AM, Virginia Wray wrote:
Hi Jack --
Thanks for reviewing. I just sent out an updated code review, so you
might want to take a look at that, but
I'll also answer your questions here....
ginnie
On 09/ 7/11 11:08 AM, Jack Schwartz wrote:
Hi Ginnie.
The bug report says that the issue is the quotes around the list of
groups, but the bugfix is more than that. Looks to me that rather
than get the list of groups from svccfg, it assumes the whole set
(the SC_VALID_GROUPS list). This indeed seems like a better check.
For the time being yes.
I don't understand the message at 519 though: SC_VALID_GROUPS
includes many groups other than "system", as defined by lines
76-81. How come the message says "system" is the only valid group?
Also, the message will be printed potentially many times as 520 is
called from a loop.
We had to minimize the allowed groupings due to all of the issues we
ran into during development, so "system" is the only valid grouping,
even
though the other groupings exist. I'm regenerated the webrev, so I'm
not sure where 520 is, but if you're referring to this:
for grping in valid_groupings:
521 err += grping + " "
522 parser.error(err)
it appends the valid_groupings to err and then prints the error
message, so the message is only printed once.
That's the area I'm referring to, but the code you quote above is the
old version, per the code review dated today 11 AM MDT. The new
version looks like this:
514 if sub_cmd[0] == CREATE_PROFILE:
515
516 # Check that the grouping specified is valid.
517 for grp in groupings:
518 if grp not in SC_VALID_GROUPS:
519 err = "system is the only supported grouping"
520 parser.error(err)
521 else:
which shows parser_error() printing the same error message possibly
multiple times. Maybe you meant to use code similar to what show on
the code review's "old" side for this block, which looks more
reasonable, something like this:
for grp in groupings:
if grp not in SC_VALID_GROUPS:
err = "Grouping must be one of "
for grping in SC_VALID_GROUPS:
err += grping + " "
parser.error(err)
Also, I notice the suggested fix in the bug report is out of date, but
maybe that's because you've iterated on this fix. (I haven't been
paying attention.)
I'll update that. Thank you for point that out.
Thanks,
Jack
Thanks,
Jack
but can you please update the evaluation with this info.
On 09/ 7/11 08:11 AM, Virginia Wray wrote:
Hi --
This is a stopper bug that I would like to get back quickly, so
that we can redeliver for build 174.
Jan has code reviewed. Can I get one more reviewer
and Dave's ok?
CR:
http://monaco.us.oracle.com/detail.jsf?cr=7087609
Webrev:
file:///net/indiana-build.us.oracle.com/data/share/vw130254/projectwork/sys-unconfig/7087609/slim_source/webrev/index.html
Thanks,
ginnie
_______________________________________________
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
