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()?
_______________________________________________
zones-discuss mailing list
zones-discuss@opensolaris.org

Reply via email to