Hi Jack,
Comments inline and including caiman-discuss since your comments are
quite useful. Also, please review the webrev at
http://cr.opensolaris.org/~clayb/AI_server/webrev_final_rc0/ tomorrow verifying
I've met your requests, so that I may try to push as soon as
possible.
Thank you,
Clay
On Tue, 14 Oct 2008, Jack Schwartz wrote:
> Hi Clay.
>
> Here's my first installment of comments. Comments on the 2 TBD files at the
> bottom are forthcoming.
>
>
> general comment: The C-style standard (used for C programs) sets indentation
> for nesting levels at one tab and statement continuation lines at 4 spaces.
> DC and snap have adhered to this indentation practice. Please do the same so
> code in the gate is consistent.
Thank you for explaining what the standard was. I've implemented this.
> criteria_schema.rng:
> -------------------
> line 69: ...is sticking out
Thank you, I've converted the whole file to use tabs. Oddly some of my vimrc's
thought this was python (thus using spaces).
> 94-117: You may want to add comments for what these different lists are for.
> This will help make this file more maintainable. In fact, I would sprinkle
> comments throughout this file to better clarify what all items and groups of
> items represent.
Thank you for pointing out XML supports comments, I've applied them through
out.
(Note: I moved the interleave tag inside the ai_criteria_manifest element, this
was a bug you didn't flag and I wondered if that was intentional?)
> ai_schema.rng:
> -------------
> 238-242: You probably based this on my manifest... I've since learned that
> mirrors are implicit in an authority, and you don't need them here. The way
> it works is that for an authority, there is a name and a preferred URL.
I took the mirror section out, I also converted the entire file to using tab's
opposed to both tabs and spaces
> webserver.py:
> ------------
>
> Please embellish with more comments to explain the code.
>
> nit: 66-67: do the os.path.exists(os.path.join(....) thing once and use the
> resultant string twice.
> 47, 48, 50, 51, 103, 106, 110-113, 125, 174-178, 184-188, 227-228, 264, 266,
> 268-271: extreme indentation
Fixed
> 165: Please add a comment about what manifest values can be.
I'd prefer if someone wonders that, that they investigate findManifest() since
the comments next to the code there should be more accurate then the comments
in the code consuming it. I will explain the digit versus string thing, seeing
your later comments and I think understanding the confusion.
> 165: Do you mean ... (manifest > "0") ? if manifest is a digit, it will
> always be > 0.
Yes, I mean to test if manifest is a digit and then if so is it greater than
zero. It may sometimes be zero however (returning the default manifest).
Otherwise, manifest is the name of manifest (the joys of Python's odd
flexibility in typing here).
> Also, is there a way to get back multiple manifests? (I assume that a
> manifest numbered > 0 will be returned not the first time, but a subsequent
> time. But I don't see a loop.
No, if we get more than one manifest (the client can only consume one manifest)
it's a bug in the database (somehow the criteria isn't collision free) or the
criteria provided isn't everything asked for.
> 171: follow on to above: do you mean elif manifest == "0" ?
No, still numeric - means we didn't get a manifest so we serve the default.
> 181: If not a digit, please clarify what else a manifest name can be by
> adding a comment.
Will do, I'll explain that the manifest is thus a string
> 171-188: This can be optimized by setting the second arg to os.path.join
> based on the value of manifest, and then having only one call to serve file?
Good eye, I've collapsed the if statement down a bit
> 226, 227, 255, 263, 265: \ not needed
Thank you, I was still quite green to Python when I wrote this not
understanding when '\' was needed obviously.
> verifyXML.py:
> ------------
>
> 1: Needs CDDL header.
>
> 17: extreme indentation.
>
> Same general comment about indentation and spaces as for files above.
>
> 35: Hmmm... I like this... maybe I'll convert my manifest checking to not
> call xmllint on the commandline, and do it as you do here...
You're welcome to import this file and use the function. That's why even though
only two calls I abstracted it out.
> publish_manifest.py:
> -------------------
>
> Please add more comments to the whole file.
Lots have been added, and a bit of refactoring too
> 1: Needs CDDL header.
>
> Same comments about extreme indentation throughout the file (lines not
> enumerated here)
>
> 22: It isn't readily clear that the files arg represents a DataFiles object.
> Please add headers and comments by each function and method explaining its
> args are, what it returns and what exceptions it can raise. This will help
> make the code easier for others to understand in the future.
I'd added some bits about returns but I'll try to go through and add required
arg types too
> 51: nit: If you use () you can get rid of the \ at the end of each line, and
> it would also make nesting clearer.
Thank you, again please flag all the newbie mistakes I've done -- I'm sure
there are lots!
> 41: The check for root seems misplaced in the options parsing function.
I've moved it into the __main__ logic.
> 58,59: Ditto for database initialization / verification.
This is part of general program setup which parseOptions does (perhaps renaming
it to setup() would be more appropriate.
> 201, 204, 485: indentation
Should have been corrected
> 214: typo: determine
Fixed
> 223: indentation should be 1 tab over from if
It is, but the parentheses are a bit thick there
> 223-225: \ not needed.
They're gone now
> 245: why not combine with line above?
Hold over from when the line was longer. Fixed in the great tabification :)
> 322: Really the error here is not writing, but opening the manifest.
Yes, but it sounds much better than "Unable able to open dest. manifest for
writing" to me. Do you read that as better though?
> 323-336: Put these in a try/except unless you have the calls to this method
> inside one already.
Thank you, good point. Except on IOError? I've left the above error text the
same and moved the except block down ~17 lines.
> 501: It will be easier for someone looking at this file in the future that
> there is a field _manifest if it is defined as None in __init__ and set in
> manifestPath. Defining all instance variables in __init__ helps to document
> them by declaring them all in one place. This is already done for several
> other instance variables; please consider doing it for _manifest and any
> others missing in __init__ as well.
I think this is the largest reason to refactor the assignment/accessors as two
functions so that these can be set to none in the __init__. I'll ensure that
the RFE gets filed to do that refactoring.
> 506: mani will never be none here
Thank you, yes that is rather the point of the if right above.
> 506-509: Please explain the logic here...
>
> listManifests.py:
> ----------------
> TBD
>
> AI_database.py:
> --------------
> TBD
>
> Thanks,
> Jack
>
>
>
>
> Clay Baenziger wrote:
>> Hi Joe,
>> Thank you for the code review! Since you're reviewing all the
>> changes I've but a webrev with all the changes start to finish at:
>> http://cr.opensolaris.org/~clayb/AI_server/webrev_complete/
>>
>> Thank you,
>> Clay
>>
>> On Thu, 9 Oct 2008, Joseph J VLcek wrote:
>>
>>
>>> I will start looking at this today.
>>>
>>> Joe
>>>
>>>
>>> Clay Baenziger wrote:
>>>
>>>> Hi all,
>>>>
>>>> I've updated the webrev, and sent out a new code tar-ball for folks to
>>>> work with.
>>>>
>>>> This update supports a "universal manifest" (based off the criteria
>>>> schema) which embeddes the A/I manifest and all SC manifests. This
>>>> universal manifest will be what is served out by the webserver for parsing
>>>> by the A/I engine. An example manifest is included as [1] here.
>>>> Unfortunately, this is a bit cludgy as it's a RelaxNG manifest which
>>>> includes XML 1.0 DTD based XML (the SMF SC manifests).
>>>>
>>>> Also, due to this new universal manifest is an updated criteria schema
>>>> allowing one to specify the A/I and SMF manifests in the criteria and
>>>> updates to publish_manifest(1) to correctly verify everything handed in.
>>>>
>>>> Lastly, due to some bugs I've found the criteria collision logic got a few
>>>> tweaks.
>>>>
>>>> Tar-ball:
>>>> http://cr.opensolaris.org/~clayb/AI_server/AI_server.finalCut.1.tgz
>>>> Webrev (just diffs off previous):
>>>> http://cr.opensolaris.org/~clayb/AI_server/webrev1/
>>>>
>>>> Thank you,
>>>> Clay
>>>>
>>>> [1]:
>>>> <ai_criteria_manifest>
>>>> <ai_embedded_manifest>
>>>> <ai_manifest name="default">
>>>> <ai_target_device>
>>>> <target_device_name>c0t0d0</target_device_name>
>>>> <target_device_type>SCSI</target_device_type>
>>>>
>>>> <target_device_vendor>hitachi</target_device_vendor>
>>>> <target_device_size>20480</target_device_size>
>>>>
>>>> <target_device_use_solaris_partition>true</target_device_use_solaris_partition>
>>>>
>>>> <target_device_overwrite_root_zfs_pool>true</target_device_overwrite_root_zfs_pool>
>>>> </ai_target_device>
>>>> <ai_device_partitioning>
>>>> <partition_action>create</partition_action>
>>>> <partition_number>1</partition_number>
>>>>
>>>> <partition_start_sector>200</partition_start_sector>
>>>> <partition_size>20480</partition_size>
>>>> <partition_type>99</partition_type>
>>>> </ai_device_partitioning>
>>>> <ai_device_vtoc_slices>
>>>> <slice_action>create</slice_action>
>>>> <slice_number>4</slice_number>
>>>> <slice_size>20480</slice_size>
>>>> </ai_device_vtoc_slices>
>>>> <ai_device_zfs_root_pool>
>>>>
>>>> <enable_mirrored_root>true</enable_mirrored_root>
>>>> <zfs_options>xyz</zfs_options>
>>>> </ai_device_zfs_root_pool>
>>>> <ai_device_swap>
>>>> <ai_swap_size>2048</ai_swap_size>
>>>> </ai_device_swap>
>>>> <ai_pkg_repo_default_authority>
>>>> <main url="http://pkg.opensolaris.org";
>>>> authname="opensolaris.org"/>
>>>> <mirror url="" authname=""/>
>>>> </ai_pkg_repo_default_authority>
>>>> <ai_packages>
>>>> </ai_packages>
>>>> </ai_manifest>
>>>> </ai_embedded_manifest>
>>>> <sc_embedded_manifest name = "AI">
>>>> <!-- <?xml version='1.0'?>
>>>> <!DOCTYPE service_bundle SYSTEM
>>>> "/usr/share/lib/xml/dtd/service_bundle.dtd.1">
>>>> <service_bundle type="profile" name="name">
>>>> <service name="ai_properties" version="1" type="service">
>>>> <instance name="default" enabled="true">
>>>> <property_group name="ai" type="application">
>>>> <propval name="username" type="astring"
>>>> value="jack"/>
>>>> <propval name="password" type="astring"
>>>> value="jack"/>
>>>> </property_group>
>>>> </instance>
>>>> </service>
>>>> </service_bundle>
>>>> -->
>>>> </sc_embedded_manifest>
>>>> </ai_criteria_manifest>
>>>> _______________________________________________
>>>> caiman-discuss mailing list
>>>> caiman-discuss at opensolaris.org
>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>>>
>>>
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>
>
>
