be_mount.c:
line 348: I'm confused as to why you do the be_get_uuid call. uu is
never used after it and
it's not clear there's any side effect. So why do so? If there's
a reason a comment would help.
line 351: Do you need the if(gen_tmp_altroot) free(tmp_altroot);
code here?
line 477: ditto
line 545: Aren't we supposed to use strncmp for security reasons?
line 598: ditto
line 645: Shouldn't these really be be_errno_t?
line 679: Nit (there always has to be at least one nit). Remove the
get, it's already on the line above.
line 828- 841: What happens if the strlcpy actually truncates dir?
line 851: Is this really none? Maybe something here or in the returns
about tmp_mp.
line 858 & 861: shouldn't this really be be_errno_t? (Actually this
is going to be a generic comment
since I see it many places)
line 867: should this be strncpy?
line 940 & 942: should this be strncmp?
line 1447: Huh?
line 1895: do we need to free altroot?
line 2175: Maybe file a bug on this?
be_utils.c
general: same comment as above for the return types. The comment
says be_errno_t but it returns an int and ret/err are ints. I
know it's really the same thing but it would be nice to see the
cleanup done sometime. Maybe a low level bug?
line 1710: continaer_ds -> container_ds
line 1754: Is there a bug open on this?
line 1755: furture -> future
line 2478: containder ->container
cleanup: I believe a close(fd) should be there.
Jean
Jean McCormack wrote:
> I'll do these tomorrow morning.
>
> Jean
>
> Joseph J VLcek wrote:
>
>> Forwarding to caiman-discuss as this had accidentally been exchanged
>> off list.
>>
>> Joe
>>
>> ------------------------------------------------------------------------
>>
>> Subject:
>> Re: Code review help
>> From:
>> Joseph J VLcek <Joseph.Vlcek at sun.com>
>> Date:
>> Mon, 06 Oct 2008 19:47:00 -0400
>> To:
>> Jean McCormack <Jean.McCormack at Sun.COM>
>>
>> To:
>> Jean McCormack <Jean.McCormack at Sun.COM>
>> CC:
>> Evan Layton <Evan.Layton at Sun.COM>, Karen Tung <Karen.Tung at Sun.COM>,
>> Jack Schwartz <Jack.A.Schwartz at Sun.COM>, Ethan Quach
>> <Ethan.Quach at Sun.COM>, Tim Knitter <Tim.Knitter at Sun.COM>
>>
>>
>> Jean,
>>
>> I've already reviewed a good hunk of usr/src/lib/libbe/be_create.c
>>
>> Could I suggest you do
>>
>>
>>> usr/src/lib/libbe/be_mount.c
>>> usr/src/lib/libbe/be_rename.c - Jean
>>> usr/src/lib/libbe/be_utils.c
>>>
>> I have already done all the Makefiles so... I will do:
>>
>>
>>> usr/src/lib/Makefile - JoeV
>>> usr/src/lib/libbe/Makefile - JoeV
>>> usr/src/lib/libbe/be_activate.c - JoeV
>>> usr/src/lib/libbe/be_create.c - JoeV
>>> usr/src/lib/libbe/be_zones.c - JoeV
>>> usr/src/lib/libbe/libbe.h - JoeV
>>> usr/src/lib/libbe/libbe_priv.h - JoeV
>>> usr/src/lib/libbe/tbeadm/Makefile - JoeV
>>>
>>
>> OK?
>>
>> Joe
>>
>>
>>
>> Evan Layton wrote:
>>
>>> Jean McCormack wrote:
>>>
>>>> Evan Layton wrote:
>>>>
>>>>> Hi Folks,
>>>>>
>>>>> We are in desperate need of help in getting our code review done so
>>>>> we're begging for help. Would any of you have some time to help out
>>>>> with this code review. I know it's rather large but we'd split
>>>>> things up so no one has to do too much of it. Please let us know if
>>>>> you can help!
>>>>>
>>>>> Joe, could you let us know how much of this you've already looked
>>>>> at so we add that in to how we split this up?
>>>>>
>>>>> Thanks,
>>>>> -evan
>>>>>
>>>> I can try. Splitting it up would definitely help.
>>>>
>>>> Jean
>>>>
>>>>
>>> A large chunk of the files listed are for the old packaging stuff and
>>> will be going away at some point. A bunch of the others are just one
>>> line changes because of the name change from libspmizones to
>>> libinstzones.
>>> These are the files that we really need to look at:
>>>
>>> usr/src/lib/Makefile
>>> usr/src/lib/libbe/Makefile
>>> usr/src/lib/libbe/be_activate.c - JoeV
>>> usr/src/lib/libbe/be_create.c - Jean
>>> usr/src/lib/libbe/be_mount.c
>>> usr/src/lib/libbe/be_rename.c - Jean
>>> usr/src/lib/libbe/be_utils.c
>>> usr/src/lib/libbe/be_zones.c - JoeV
>>> usr/src/lib/libbe/libbe.h
>>> usr/src/lib/libbe/libbe_priv.h
>>> usr/src/lib/libbe/tbeadm/Makefile
>>>
>>> These are mostly just changes around the name change from
>>> libspmizones to libeinstzones:
>>> usr/src/lib/libinstzones/Makefile
>>> usr/src/lib/libinstzones/instzones_api.h
>>> usr/src/lib/libinstzones/instzones_lib.h
>>>
>>> usr/src/lib/libinstzones/zones.c
>>> This file has most if not all of the changes in this
>>> library needed to support libbe.
>>>
>>> usr/src/lib/libinstzones/zones_args.c
>>> usr/src/lib/libinstzones/zones_exec.c
>>> usr/src/lib/libinstzones/zones_locks.c
>>> usr/src/lib/libinstzones/zones_lofs.c
>>> usr/src/lib/libinstzones/zones_paths.c
>>> usr/src/lib/libinstzones/zones_states.c
>>> usr/src/lib/libinstzones/zones_str.c
>>> usr/src/lib/libinstzones/zones_strings.h
>>> usr/src/lib/libinstzones/zones_utils.c
>>>
>>> These changes are simply the changes needed to deliver beadm in it's
>>> own package to make back porting a bit easier
>>>
>>> -- JoeV --
>>> usr/src/pkgdefs/SUNWbeadm/Makefile
>>> usr/src/pkgdefs/SUNWbeadm/depend
>>> usr/src/pkgdefs/SUNWbeadm/pkginfo.tmpl
>>> usr/src/pkgdefs/SUNWbeadm/prototype_com
>>> usr/src/pkgdefs/SUNWbeadm/prototype_i386
>>> usr/src/pkgdefs/SUNWbeadm/prototype_sparc
>>>
>>>
>>> The rest of the changes listed are not all that important to have
>>> folks like at since they are just the header file name change from
>>> libinstzones.
>>>
>>> For now since others haven't had a chance to respond yet:
>>> Jean can you look at be_create.c, be_rename.c and libinstzones/zones.c
>>> JoeV can you look at be_activate.c, be_zones.c and the pkgdef files
>>> for SUNWbeadm.
>>>
>>> We'll assign other files as we get others to help.
>>>
>>> Thanks!
>>> -evan
>>>
>>>
>> ------------------------------------------------------------------------
>>
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>
>>
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
