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
676. Also no need to init on line 600.
616-619: Why not init beName to None
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
comment 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
improve 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?
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?
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-discuss
>
