Ed,

My responses are in-line.

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/

Fixed.

> ----------
> 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)

This was old code but I went ahead and changed it as you suggest.

> - why does: zonecfg_add_patch_obs() take a zone_dochandle_t?

I had been thinking about keeping the signatures on the related
functions consistent but there was no real reason for this so
I fixed it.

> - in zonecfg_add_patch_obs(): why is cur a "void *" instead of a
>   xmlNodePtr?

Some of the consumers of this library don't know about xml.  In
particular the stc which I don't want to break.

> - 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...)

The zonecfg_call_zoneadmd calls within sw_support bring in these
dependencies.

> - perhaps the comment:
>       .../var/run/zones/<zonename>.zoneadm.lock
>   should be changed to:
>       .../var/run/zones/<zonename>.libzonecfg.lock

I think the name is still ok since all of this activity
still flows from an invocation of zoneadm, even though
this code has moved to libzonecfg.

> - 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);

This was existing code that I just moved but I went ahead
and added this.

> - 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);
>       }
> 
>       ...

OK.

> - 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()?

This is existing code that I just moved, but it looks like it mallocs
a page to use as the return buffer for the door call.

Thanks again for your comments.  I'll roll these in with the other
changes for an updated webrev.

Jerry

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

Reply via email to