Alok,
Alok Aggarwal wrote:
> On Mon, 8 Dec 2008, William Schumann wrote:
>
>
>> Alok,
>> I added TB and GB units and tested and updated the webrev. Thanks
>> William
>> Alok Aggarwal wrote:
>>
>>> On Tue, 2 Dec 2008, William Schumann wrote:
>>>
>>>
>>>> Hey Alok,
>>>> I have been reading the AI XML parsing code. Is there an easy way of
>>>> launching the validator from a development system like indiana-build,
>>>> or do I need to do this on a client?
>>>>
>>>> I read a comment somewhere about using the DC validator in the
>>>> future. Anything new on this?
>>>>
>>>> I've been using the Jing validator (seems to work OK, uses Relax NG
>>>> schema file, but doesn't know about .defval.xml, I think) and the
>>>> xmloperator editor (supposed to validate against rng file, but buggy
>>>> for me).
>>>>
>>>> Also, xmloperator prepended:
>>>> <?xml version="1.0" encoding="UTF-8"?>
>>>> to ai_manifest.rng. It seems logical that we use UTF-8, since we're
>>>> international, but perhaps our character set for the manifest still
>>>> shouldn't take anything other than ASCII. What do you think? Probably
>>>> not so important, eh?
>>>> William
>>>>
>>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=4460
>>>> http://cr.opensolaris.org/~wmsch/bug-4460/
>>>>
>>>> Added ability to specify partition size units of either sectors or
>>>> megabytes.
>>>> Created new Relax NG element: partition_size_units, defaulting to
>>>> megabytes
>>>>
>>> So, I think in general we should also allow
>>> gigabytes and terabytes as the units since they're
>>> quite valid. Does it make sense to get rid of
>>> the sectors notation entirely? I mean do people
>>> really prefer this over mb, gb or tb?
>>>
>>> I also think applying this extension to slices
>>> at this time makes sense, it sounds kinda odd
>>> that partition units can be specified but slice
>>> units can't be.
>>>
>>> auto_install.c: line 310: I haven't looked at this
>>> closely but don't you want something like MB_TO_BLOCKS here?
>>> Or, is just a matter of naming the define that instead of
>>> BLOCKS_TO_MB?
>>>
>> You're right. There shouldn't be a constant here, but a formula.
>>
>>> auto_parse.c: Why not just name the function
>>> ai_get_manifest_partition_units() to match what the
>>> rest of the code does?
>>>
>> Yeah, it's since there is so much code duplication. I wrote a generic
>> function that just took the element as a parameter. Is there a
>> particular reason why you broke it up into so many different functions?
>> If I can get you to see it my way, I'll make them all use the same
>> function. If you win, I'll change the new function name as you want. :)
>>
>> Thanks for this little pre-review. :)
>> wm
>>
>
> ai_manifest.rng: line 1, 24, 31: Why are these changes
> needed?
>
xmloperator added these once when I edited it. Line 1 in particular
seems useful, clearly specifying UTF-8 encoding so that the user and
the parser are speaking the same language. I restored lines 24 and 31,
but do you think that specifying UTF-8 should be kept?
> ai_manifest.rng: Have you verified that if you specify
> any other value for the partition_size_units field, it
> fails during the validation phase?
>
Yes
> ai_manifest.xml: line 53: move this one line up so it's
> next to the partition_size field.
>
> auto_install.c: line 49, 50, 305: const not needed
>
> auto_install.h: line 75: CONVERT_UNIT_TO_TEXT may be more
> readable.
>
>
Made above 3 changes.
> auto_parse.c: line 405, 410, 415, 420: How are the capitalized
> versions even allowed given your schema?
>
>
Indeed, they are not! And there doesn't seem to be a simple way to get
the parser to recognize them as being case-insensitive, so I'm adding
uppercase options. Datatype libraries are suggested as a solution to
this, and it might be advisable if these units were used in a number of
places in the future.
Resubmitted webrev.
William
