Ethan,
On 04/12/10 06:38 PM, Ethan Quach wrote:
Comments inline; also had a couple of additional comments
for the updated webrev.
On 04/09/10 07:23, William Schumann wrote:
Ethan,
On 04/ 9/10 02:00 PM, Ethan Quach wrote:
ai_manifest.rng - 375 - A nit on the new tag name. Why not just
name it "on_existing"? That's not consistent with existing
tagging convention, but we need to change that anyhow.
So that users are not confused with inconsistent usage. When the
tagging convention changes, it should change uniformly.
I don't see how this is a confusion point. We're
defining a new element, so we need to add new
documentation for it anyway; whatever we define
and document is what users use.
A user composing a manifest is likely to see a pattern in the "slice_"
prefix and to think that there has been a mistake if it is missing,
perhaps in the documentation or implementation, or perhaps user error.
(A computer would have no problem with this.) Composing an XML manifest
by hand is not intuitive, and it is helpful to be consistent in the
grammar to reduce doubt.
auto_parse.c - 744-746 - For completeness, could you expand
this to an if/else clause that compares for all possible string
values here.
Typically, this is not done, since the code is unnecessary and would
change nothing, while consuming various resources. I might advocate
documenting code for completeness, but not coding for the sake of
completeness.
It wouldn't change anything, but makes it more tolerant
of the definitions of what the possible value are. The code
currently relies on the fact that the definition for the value
we want there by default is equal to the value in the
initialized array structure, '0'. Could you at least
comment that then.
I can mention this here, but it is a practice throughout auto_parse.c -
zero is always the default.
auto_parse.c - 365, 397 - does changing this impact the
max values users can input for the sizes of swap and
dump? I noticed that the rng defines the values to be of
type unsignedLong.
I will back out this change to code that was flagged by lint. The
values for swap and dump sizes have mixed signed/unsigned representation
here, and elsewhere. This is related to bug 15568 "Checking unsigned
int for value < 0 in liborchestrator" and the fix should address them as
a whole.
ai_manifest.rng - 461-463, 479-481 - I must have not
noticed this before you merged the webrev with the fix
for 7794, but defining this new tag as optional for the
delete and preserve slice_actions seem unnecessary. Is
there a reason? The premise for the fix described in the
bug report states we're introducing a new optional tag
only for the create slice_action.
The reason for the new tag being optional is the same as for all of the
other slice and fdisk partition tags being optional (when not
required): they are supplied in the default values file,
ai_manifest.defval.xml, so that are always present on the AI client.
This technique becomes necessary due to a deficiency in ManifestServ,
which cannot combine associated data. If it were not given a default,
the missing values could not be mapped to their corresponding actions
(in auto_parse.c). The default values pad the "C" arrays of slice values
with defaults. Also, since ai_manifest.defval.xml is not processed on
the AI server side, the defaults are not present; so the schema must be
flexible enough to validate the manifest when values are there and when
they aren't.
This was described in detail in bug 7794,
http://defect.opensolaris.org/bz/show_bug.cgi?id=7794
Also, although slice actions can be handled separately in the schema,
the associated tags' default values cannot (to my knowledge) be handled
separately in the default values file.
Updated webrev.
William
On 04/08/10 09:28, William Schumann wrote:
http://cr.opensolaris.org/~wmsch/bug-8661/
http://defect.opensolaris.org/bz/show_bug.cgi?id=8661
Added tag to AI manifest RNG schema ai_manifest.rng for slices:
slice_on_existing. Possible values:
"error" (default) - if a slice N already exists and is specified in
a slice create action, consider it a configuration error and
terminate the install to protect the slice.
"overwrite" - overwrite the existing slice N definition by first
deleting it in AI's internal representation of the VTOC, then
effecting the new slice creation.
Added new parameter to om_create_slice() to indicate the option.
Added default value to ai_manifest.defval.xml as array placeholder
(the usual workaround).
Implemented as an enum to facilitate addition of other options in
the future.
Cleaned up some old lint in auto_parse.c.
Testing: created error conditions, checked behavior of overwriting
slice, multiple slices, tested proper manifest validation, tested
defaulting.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
