Thanks Jack,
If I didn't make a comment below then I implemented your suggestion.
>
> Hi everyone.
>
> Here are my comments for this round:
>
> BootEnvironment.py:
>
> This is the right idea,but the the stuff added
> (55-57, 68-69, 79) are
> constants. They probably should be classbound. No
> need to create new
> sets of these for each instance. To make them
> classbound, initialize
> them outside of __init__. Then refer to them as
> classname.varname.
> e.g. BEMaxWidths.DATASET vs beWidth.DATASET
>
> beadm.py:
> ---------
> Things look much better. Thanks for adding headers
> for public functions.
>
> 250: Nit: I suggest making a general question
> function, but OK...
>
> 397, 401: optimization: save len(beName_mntPoint) in
> a variable and
> refer back to at these locations. No need to call it
> three times for
> the same result.
>
> 436: len(opts) != 1
>
> 478: len(BeNames) == 0 is redundant with len(beNames)
> != 2
>
> 609: pool isn't used before it is overwritten either
> on line 640 or
It could be used at 685 without it being set at 609.
> 676. Also no need to init on line 600.
If it is not initialized a traceback could be produced on line 685.
>
> 616-619: Why not init beName to None
beName is passed in. Would overwrite value.
>
> 866-867 Rather than [idx-2] or [idx+4] it would be
> clearer to say what
> the proper value is using the constants. E.g. where
> idx =
> beWidth.STATUS, say [beWidth.ACTIVE] or
> [beWidth.MOUNTED] (or if you use
> the classbound names that would be BEMaxWidths.ACTIVE
> or
> BEMaxWidths.MOUNTED). This will make the code more
> understandable.
>
> Assuming beWidth is of the class BEMaxWidths, doing
> the above you'd see
> that beWidth.MOUNTED was left out on line 57 too...
> Disregard this
> omment if I am incorrect.
>
> 845, 821,807,895, 918,939, 990-991,1009-1010: Sim to
> 866-867, though not
> quite as important as the index is used by itself.
> Still it would be
> mprove code readability though...
>
> messages.py:
> ------------
> I don't see any changes related to my suggestions
> here, despite emails
> that things had been fixed. Were these changes lost
> in a merge perhaps?
>
I'm going to have to address this as a bug so it won't be done for this code
review.
> libbe_glue/Makefile:
> ---------------------
> Changes look fine.
>
> libbe_glue.c:
> -------------
> 153, 159: nvlist_free(beAttrs) missing. May also
> need to check for NULL
> first.
>
> beCopy: Error returns are perhaps the only place
> where a goto is called
> for, and this function could use one. Instead of
> duplicating these
> three lines:
>
> nvlist_free(beAttrs);
> nvlist_free(beProps);
> return (Py_BuildValue("i", 1));
>
> all over the place, use a goto in their place, and
> add the goto label
> and these lines as the last four lines of the
> function. The code will
> be condensed, and thus more readable.
>
> 207: return (Py_BuildValue("", NULL));
>
> beList: Ditto the comments made for using a goto for
> error handling.
>
> beActivate:
> 315: The variable "tuple" has a deceptive name as it
> returns an integer
> object not a tuple.
> 318: return (Py_BuildValue("i", 1);
>
> Throughout the module: stuff returned by
> Py_BuildValue() needs to be
> Py_DECREF()ed if an error is returned from the
> function which called
> Py_BuildValue() and the created object is to be
> thrown away. (Recent
> email exchanged with Tim on this won't be reflected
> in this code review,
> however.) ... may be better to stick with
> PyString_FromString instead of
> using Py_BuildValue?
If an error happens in Py_BuildValue() then execution is halted and one object
is not Py_DECREF()ed which will be garbaged collected anyway. Also since the
value passed to Py_BuildValue() is guaranteed to be not NULL the only
legitimate errors that can happen are Python internal errors. Also in order
to Py_DECREF() the object it has to be set to a variable and that variable
passed in and Py_DECREF()ed on failure. The ugliness of that is not worth
checking for the failure. So I'm not going to make this change. I did however
change the Py_BuildValue() calls to PyString_FromString() since errors from
PyString_FromString() are handled better.
Thanks again Jack
Tim
>
> beDestroy: 355, 358: same comments as for
> beActivate.
> beDestroySnapshot: 397, 400: same comments as for
> beActivate.
> beRename: 441: same comment about the name "tuple"
> beMount: 486: same comment about the name "tuple"
> beUnmount: 528: same comment about the name "tuple"
> beRollback: 572: same comment about the name "tuple"
>
> beDestroySnapshot: Comments similar to beActivate().
>
> nvlist_alloc_w() is never called and can be removed.
>
> libbe_glue.h:
> -------------
> File looks fine.
>
> Ethan Quach wrote:
> > Hello again caimaniacs,
> >
> > Thanks for those that provided comments for the
> early code review.
> > Posted here is the incremental webrev of the
> changes for code review
> > comments plus other fixes that we've pushed since.
> >
> >
> http://cr.opensolaris.org/~equach/webrev.SnapUpgrade.2
> >
> >
> > Additionally as a reference, below is the webrev of
> the overall snap
> > upgrade gate synced up against the current
> slim_source base.
> >
> >
> http://cr.opensolaris.org/~equach/webrev.SnapUpgrade
> >
> >
> > Again comments welcomed, particularly from those
> who provided the
> > initial comments on the early review.
> >
> >
> > Thanks,
> > -Ethan
> > _______________________________________________
> > caiman-discuss mailing list
> > caiman-discuss at opensolaris.org
> >
> http://mail.opensolaris.org/mailman/listinfo/caiman-di
> scuss
> >
>
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-di
> scuss
--
This message posted from opensolaris.org
