Edward Pilatowicz wrote:
> On Wed, May 21, 2008 at 09:43:27AM -0600, Jerry Jelinek wrote:
>> Thanks again for your input.  I am rebuilding and retesting with
>> these changes.  Once that is done, I'll post an updated webrev.
>>
> 
> i couldn't wait.  :)
> more comments are below.
> ed
> 
> ----------
> usr/src/lib/libbrand/common/libbrand.c
> 
> - ok
> 
> ----------
> usr/src/cmd/zoneadm/zfs.c
> 
> - this comment doesn't seem right:
>       Run the brand's post-snapshot hook before we take a ZFS snapshot
>   i think you want to s/before/after/
> 
> ----------
> usr/src/lib/libzonecfg/common/libzonecfg.c
> 
> - nit: "void *" pointers are evil.  could you redefine:
>       static void *g_devwalk_data
>   as
>       typedef struct __g_devwalk_data *g_devwalk_data_t;
>       static g_devwalk_data_t g_devwalk_data;
>   (for details about why to use the above syntax see PSARC/2004/413)
> 
> - why does: zonecfg_add_patch_obs() take a zone_dochandle_t?
> 
> - in zonecfg_add_patch_obs(): why is cur a "void *" instead of a
>   xmlNodePtr?
> 
> - why are you moving all the zoneadm locking and door code into zonecfg?
>   (i can't find anyone else other than zoneadm calling this stuff...)
> 
> - perhaps the comment:
>       .../var/run/zones/<zonename>.zoneadm.lock
>   should be changed to:
>       .../var/run/zones/<zonename>.libzonecfg.lock
> 
> - in zonecfg_grab_lock_file() could you:
>       assert(zone_lock_cnt >= 0);
>       assert(getenv(LOCK_ENV_VAR) != NULL);
>       if (zone_lock_cnt > 0) {
>               assert(atoi(getenv(LOCK_ENV_VAR)) == 1);
>       }
>       assert(atoi(getenv(LOCK_ENV_VAR)) == 0);
> 
> - nit: in start_zoneadmd(), instead of:
>       if ((child_pid = fork()) == -1) {
>               zperror(gettext("could not fork"));
>               goto out;
>       } else if (child_pid == 0) {
>               ...
>               _exit(1);
>       } else {
>               ...
>       }
>   how about:
>       if ((child_pid = fork()) == -1) {
>               zperror(gettext("could not fork"));
>               goto out;
>       }
> 
>       if (child_pid == 0) {
>               ...
>               _exit(1);
>       }
> 
>       ...
> 
> - nit: we have direct bindings now. :)  so why bother with:
>       _exit(1)
>   instead just call:
>       exit(1)
> 
> - um, why does zonecfg_call_zoneadmd() need to call getpagesize()?

Ed,

Thanks for the additional comments.  Some of this code is pre-existing
code that just got shuffled around during this re-organization.  I'll
take at look at each of these to see if I want to change them or leave
the code as-is, since that is how it has been.  I'll then respond on each
of your points.

Thanks again,
Jerry

_______________________________________________
zones-discuss mailing list
zones-discuss@opensolaris.org

Reply via email to