Evan,
Thanks for reviewing. Responses below.
On 08/10/10 00:33, Evan Layton wrote:
Hi Ethan,
I've looked mostly at the C files.
auto_ddu_lib.c
===============
nit: line 745
The variable name changed from noinstalls to actions. However
noinstlen did not get renamed amd it could be a bit confusing what
this len references if it's not also changed to something like actslen.
Agreed. Will fix.
auto_parse.c
===============
very minor nit: The comment on line 449 states "In the new schema"
however going forward this is just "the schema" :) Would it be better
to say "In this Schema" and "In the previous schema"?
Agreed. Will fix.
yet another nit:
In ai_get_manifest_disk_info (lines 413 to 439),
ai_get_manifest_swap_device_info (lines 563 to 594 and
ai_get_manifest_dump_device_info (lines 614 to 644)
This lines all appear to be doing very similar thing. While it's
obvious that all three function are needed I'm wondering if there is
any way to factor out this seemingly common code so it's only in one
place? If not just ignore this and move on. ;)
As I understand things, this code is all due to be significantly modified,
if not totally re-written, when AI is refactored under CUD. So, I don't
believe it makes sense to tidy up things like this, especially as there
is the
risk, however small, that regressions might be introduced while doing so.
line 1020: I know this isn't part of your changes but this isn't
checking that the calloc is successful and should have a check for a
return of NULL after the calloc.
Agreed. Will fix.
This is also more of a nit but I noticed throughout the C code was
that pointers tend not to be initialized to NULL. While this is not a
huge deal it can cause issues if things are not handled properly.
For example in auto_parse.c lines 997 and 998 it might be good to do:
997 auto_slice_info *asi = NULL;
998 auto_slice_info *tmp_asi = NULL;
Once the test for NULL after the calloc() has been added, as you
suggest, then
there is no danger of these pointers being accessed incorrectly.
Thanks,
- Dermot
Other than these minor nits the code looks really good.
-evan
On 8/5/10 4:07 PM, Ethan Quach wrote:
Hi all,
The following is the code review for the AI manifest schema changes,
and the installadm criteria changes. It is a rather large review, so
partial/piecewise review would be also be fine, just let us know what
you're reviewing. We've pre-requested reviews from some of you
already, but all comments welcomed by Aug 16th.
Webrev:
-------------
http://cr.opensolaris.org/~equach/webrev.ai-schema/
Bugs:
--------
16423 <http://defect.opensolaris.org/bz/show_bug.cgi?id=16423> Updates
to AI schema should be made
15449 <http://defect.opensolaris.org/bz/show_bug.cgi?id=15449>
installadm add validates combined manifest against image-specific schema
as well as schema in /usr/share/auto_install/
6975043 <http://bugs.opensolaris.org/view_bug.do?bug_id=6975043>
separate criteria and ai manifest
thanks,
-ethan
_______________________________________________
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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
