Jack,
Thanks for the code review. Comments are inline.
Jean
Jack Schwartz wrote:
> Hi Jean.
>
> Thanks for your efforts. Here are my comments:
>
> DC-manifest.defval.xml (DC_bugs3)
> ---------------
> OK
>
> DC-manifest.rng:
> ---------------
> 223: What is a suitable default for the build area? We can add a
> comment here,
> and then add stuff to the defval manifest and defaults module to set
> it up.
> How about /export/<distro_ID>?
We had talked about it not having a default. The user MUST specify a
build area.
Having a default is unsafe since multiple people are more likely to end
up with the
same area that way. i.e. default name and default build area would be
the same.
>
> 437: This line looks the same as the original. Did you tab over, or use
> spaces? Tabs are better.
its -> it's
>
> DC_checkpoint.py:
> ----------------
> 203-204: nit: arguments are listed in a different order.
Fixed.
>
> 305: Does the invalid name get printed out, so the user knows which
> name to
> change? (Name and number would probably be best.)
Yes.
print "An invalid step (%s) was specified." % name
We've gotten away from printing both per Frank's comment.
>
> 364: This:
> for step in cp.step_list[:laststep+1]:
> can be reduced to this:
> for step in cp.step_list:
I should add a comment to make this clearer. laststep is the number
of the last resumable step. So step_list[:laststep+1] is all steps from
the beginning up until and including the last resumable step.
>
> 419, 431: Nit: I'd like to see the word "step" as well as the "===="
> here.
> This may prove helpful in parsing the output.
This was decided upon in the email exchange about the -q (now -l) output.
The general idea is that both aren't really needed and in fact most
people will
use names because they 1) don't change and 2) are easier to remember which
step does what.
>
> 441 - 451: This is a mismerge with my changes. Please use what is in
> slim_source.
Sorry. Changed.
>
> 692: Why barf here? Just print script number instead, or just blank
> out the
> checkpoint_name and let the checkpoint_message or script name print by
> itself.
Again, it was decided to make the checkpoint name required. I know
Imentioned this
on the phone before you went on vacation.
>
>
> DC_defs.py:
> ----------
> Please add a comment at the beginning of the wad of manifest field
> defs, and
> another at the beginning of other defs (86+), to distinguish one group
> from the
> other. Feel free to add more comments as you see fit.
Done.
>
> DC_ti.py:
> --------
>
> 122-132: Looks to me that BOOTROOT, TMP and PKG_IMAGE areas are being
> created
> under mntpt, not in the build_data dataset (which the comments at 74
> and 118
> say).
TMP is defined as build_data/tmp so mntpt + TMP does what the comment says.
Same for bootroot and pkg_image.
>
> 162: No need for the else here, since the "if" clause returned
> unconditionally.
> This buys you one nesting level.
Great. Thanks. Changed.
>
> 167: Instead of:
> if cp.get_zfs_found():
> do lots of nested stuff
> do this:
> if (not cp_get_zfs_found()):
> return
> do lots of nested stuff but at one less nesting level.
It's not that simple. The code does this:
if cp.get_zfs_found()
do lots of nested stuff
do other stuff
And the do other stuff needs to be after the do lots of nested stuff
because in some cases it falls through.
>
> 172: How come this isn't strip()ped? (use strip(), not rstrip())
I believe it wasn't needed but it shouldn't hurt anything to have it
there and could help.
I'll put strip() back on.
>
> 190-191: Maybe I'm being dense, but I don't understand difference
> between mntpt
> and build_area. Please clarify. Better yet, add a comment.
You're not dense I'll add a comment.
mntpt is always a mount point. build area can be either a dataset or a
mountpoint depending
upon what the user specified.
>
> 204: I don't understand this either. Is mntpt an empty string, None, ???
> I think this is when zfs is found but the provided mount point is not
> zfs but
> I'm not sure why... Please add a comment.
>
> Please add a comment at all of your "elses". This will help make the
> code more
> readable / understandable. I'll also be able to review it better...
I'm going to add lots of comments to this method to address the confusion.
This elif mntpt: is overly cautious. It would serve just as well to be
an else:
or even indented since the if clause always returns.
I'll change it so that the code isn't overly confusing.
>
> 267: Nit: I'd prefer to see execfiles of definitions files toward the
> top of a
> file instead of at the bottom, since they are kinda-sorta like include
> files.
>
Done.
> DC_tm.py:
> --------
> OK.
>
> DefaultsModule.py:
> -----------------
> Is logdir (line 67) still used? If not please remove.
>
No it's not. I actually just found this when doing the mirroring bugfix.
> ai_bootroot_configure:
> ---------------------
> From your and Karen's responses to my bootroot autosize code review, I
> anticipate that we will be keeping separate args for bootroot and temp
> dirs,
> not a single arg for the build area. If that's the case, then there's
> no need
> to make changes to this file.
>
> After talking with you and synching up, we've agreed that we will be
> keeping
> separate args for these things, and so I will only give cursory review
> to the
> files which are being changed, but which I will be changing back with my
> putback. I will mark these files / changes as "cursory/temp".
>
> All changes in ai_bootroot_configure are cursory/temp.
>
> ai_x86_image.xml:
> ----------------
> OK.
>
> dc_utils.py:
> -----------
> 54: On the surface, I don't agree with this change. An empty string
> is still a
> string. How come this change is necessary?
An empty string is the same as None though. It imparts no information.
So instead of code that does
value = get_manifest_value()
if value is None or len(value) == 0:
I can just do:
if value is None:
Is there ever a case where we would want the empty string?
>
> distro_const.py:
> ---------------
> OK.
>
> Line 324 is cursory/temp/OK.
>
> finalizer_checkpoint.py: (DC_bugs3)
> ------------------------
> Cursory/temp/OK
>
> finalizer_rollback.py: (DC_bugs3)
> ---------------------
> Cursory/temp/OK
>
> slim_cd.xml:
> -----------
> OK
>
> slimcd_bootroot_configure:
> -------------------------
> Cursory/temp/OK
>
> slimcd_post_bootroot_pkg_image_mod:
> ----------------------------------
> Cursory/temp/OK
>
> slimcd_pre_bootroot_pkg_image_mod:
> ---------------------------------
> OK
>
> bootroot_archive.py:
> -------------------
> Cursory/temp/OK
>
> bootroot_configure:
> ------------------
> Cursory/temp/OK
>
> bootroot_initialize.py:
> ----------------------
> OK
>
> create_iso:
> ----------
> Cursory/temp/OK
>
> create_usb:
> ----------
> 47: I know we've been doing this for a while, but...
>
> I caution against using /tmp as a work area, especially knowing
> usbgen will use it to store both an iso and the usb image. This puts a
> requirement that the system has lots of memory. If the system doesn't
> have the
> RAM it will need, a swapfile will have to be set up, and users may not
> know
> about that. They'll only see an error :(
>
> I encourage the use of a temp area under the build area instead, since
> not all
> systems have gigabytes of RAM, disk space is cheap, the temp area will
> be in
> the same general vicinity as everything else used to build the distro
> for quick
> inspection/debugging (if we don't clean it up), and we can tidy
> everything up
> nicely (if we do clean it up).
>
> I suppose for now we can leave this, but I'll file a low priority bug
> to keep
> this on the radar for future discussion.
>
My original code had the usbgen work area as the build_data/tmp directory.
The creation of the usb image from the iso took an hour, or more. If we
use /tmp it
takes much much less. That's why I made this change. We should think about
what we want to do. It's easy to modify.
> post_bootroot_pkg_img_mod:
> -------------------------
> 84: please use tabs
Done.
Thanks,
Jean
>
> Rest is Cursory/temp/OK
>
> pre_bootroot_pkg_img_mod:
> -------------------------
> Cursory/temp/OK
>
> Thanks,
> Jack
>
> Jean McCormack wrote:
>> I'd like Jack, when he gets back, and Karen to look at this:
>>
>> Webrev:
>> http://cr.opensolaris.org/~jeanm/DC_bugs/
>>
>>
>> bugs fixed:
>> http://defect.opensolaris.org/bz/show_bug.cgi?id=3357
>> http://defect.opensolaris.org/bz/show_bug.cgi?id=3359
>> http://defect.opensolaris.org/bz/show_bug.cgi?id=3467
>> http://defect.opensolaris.org/bz/show_bug.cgi?id=3555
>> http://defect.opensolaris.org/bz/show_bug.cgi?id=3556
>> http://defect.opensolaris.org/bz/show_bug.cgi?id=3557
>> http://defect.opensolaris.org/bz/show_bug.cgi?id=3560
>> http://defect.opensolaris.org/bz/show_bug.cgi?id=3647
>>
>> Jean
>>
>>
>>
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>
>
