Hi Glenn.
Nice job at managing all of this. Here are my comments for the first
code review:
beadm.py:
Both gettext.install() calls are removed. Isn't one of them
(or maybe _.install()) needed?
be_list.c:
zone_be_contianer_ds is a typo in many places.
be_mount.c:
1745: Can get rid of the lint error by applying DeMorgan's law and
inverting all operators. This is cleaner than just putting /* LINTED */
to cover up the issue.
!(a && b && (c || d))
!a || !b || !(c || d)
!a || !b || (!c && !d)
if (p1 != NULL &&
strncmp(p1 + 1, BE_CONTAINER_DS_NAME, 4) == 0 &&
/* LINTED statement has no consequent */
(*(p1 + 5) == '/' || *(p1 + 5) == '\0')) {
;
} else {
else_stuff();
}
becomes
if (p1 == NULL ||
strncmp(p1 + 1, BE_CONTAINER_DS_NAME, 4) != 0 ||
(*(p1 + 5) != '/' && *(p1 + 5) != '\0')) {
else_stuff();
}
be_utils.c:
2304/2305: can be condensed to:
(void) vfprintf(stderr, prnt_str, ap);
libbe.h:
It makes more sense to me to put lines 29-31 below the #include lines of
33-35. Each #include line brings in the contents of a file, which
itself will have
#ifdef __cplusplus.
Nesting these #ifdefs (through the includes) seems like unnecessarily
asking for trouble.
libbe_priv.h:
Same comment as for libbe.h
tbeadm.c:
How come tbeadm.c is getting deleted? Is it being replaced by the
official tests done by Harold?
Review of second wad is forthcoming.
Thanks,
Jack
On 07/12/10 05:05 PM, Glenn Lagasse wrote:
[re-sending as obviously it would help if there was a webrev for people
to review]
The ON SNAP Integration project is ready for a code review. Since we're
moving existing code in slim_source into ON I've tried to make things a
little easier to review.
I have two webrevs/workspaces for review. The first workspace is based
on slim_source and is sync'ed with Jack's latest changes that he's
already got a review out for. This workspace has the file differences
for the existing files that are moving from slim_source into ON. This
workspace should be used to review only files that aren't "new"
as part of the integration into ON*. That list of files is:
usr/src/cmd/beadm/BootEnvironment.py
usr/src/cmd/beadm/__init__.py
usr/src/cmd/beadm/beadm.py
usr/src/cmd/beadm/messages.py
usr/src/lib/libbe/be_activate.c
usr/src/lib/libbe/be_create.c
usr/src/lib/libbe/be_list.c
usr/src/lib/libbe/be_mount.c
usr/src/lib/libbe/be_rename.c
usr/src/lib/libbe/be_snapshot.c
usr/src/lib/libbe/be_utils.c
usr/src/lib/libbe/be_zones.c
usr/src/lib/libbe/libbe.h
usr/src/lib/libbe/libbe_priv.h
usr/src/lib/libbe_pymod/libbe_py.c
usr/src/pkg/manifests/install-beadm.mf
The webrev for this is:
http://cr.opensolaris.org/~glagasse/hg_beadm_1_noinstallgrub/
* Note: This workspace/webrev does *not* contain all of the changes to
actually remove beadm/libbe from slim_source. That will be done
seperately.
The second workspace is based on the ON gate and is the 'final' workspace
which includes all of Jack's changes as well as my changes for the project.
This workspace should be used to review only files that are "new" as part
of the integration (although you can also look at it for a feel of what is
going where once everything is transferred). That list of files is:
added:
usr/src/cmd/beadm/Makefile
usr/src/cmd/beadm/__init__.stub.py
usr/src/lib/libbe/Makefile
usr/src/lib/libbe/Makefile.com
usr/src/lib/libbe/common/llib-lbe
usr/src/lib/libbe/common/mapfile-vers
usr/src/lib/libbe/i386/Makefile
usr/src/lib/libbe/sparc/Makefile
usr/src/lib/pylibbe/Makefile
usr/src/lib/pylibbe/Makefile.com
usr/src/lib/pylibbe/common/mapfile-vers
usr/src/lib/pylibbe/i386/Makefile
usr/src/lib/pylibbe/sparc/Makefile
usr/src/pkg/manifests/SUNWbeadm.mf
modified:
exception_lists/interface_check
usr/src/Makefile.lint
usr/src/Targetdirs
usr/src/cmd/Makefile
usr/src/lib/Makefile
usr/src/lib/libsecdb/exec_attr.txt
usr/src/lib/libsecdb/prof_attr.txt
The webrev for this is:
http://cr.opensolaris.org/~glagasse/onnv-clone
I'd like to get comments no later than noon on Friday July 16th. If you
need longer, please let me know. Also please drop me a line as soon as
possible if you plan to review these changes (if you've already told me
you will, no need to do this obviously).
Thanks!
Glenn& Jack
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
