On Thu 07 May 2009 at 10:38AM, Danek Duvall wrote:
> On Thu, May 07, 2009 at 02:11:17AM -0700, Dan Price wrote:
>
> > > >http://cr.opensolaris.org/~dp/pkg-zonefixes3/
> >
> > Webrev is reposted
>
> Looks fine to me; just a few nits.
>
> attach:
>
> - line 425: is GZ_IMAGE something you'd expect to set from outside the
> script? If not, I'm not sure I understand its purpose.
In attach it is a parallel to GZ_IMAGE in pkgcreatezone. It's
essentially a hook for a developer (me) to be able to reset what
the notion of the global zone is. I can take it out, but it's
harmless, and was amazingly usefull in my testing, where I just
edited it to be /tmp/somethingelse, an image representing a different
global zone configuration. I would like to keep it.
> - line 455: You might have a comment here saying you're resetting
> PKG_IMAGE now that you're done collecting information. You also don't
> need the second export call, though it won't hurt.
Added.
> - Is it worth checking that either both the key and the cert are None or
> neither is?
I don't think pkg(5) enforces that, so I didn't either.
> common.ksh:
>
> - line 163,166: no need for backslashes. Or square brackets around the
> single space (did you mean for that to be space and tab?).
Ok.
> - line 164: funny indentation (space tab space)
Fixed.
> pkgcreatezone:
>
> - line 260 et al: you can put the $() construct inside the double quotes,
> which will look more natural.
Ok.
> - line 275: You could create the image with --no-refresh earlier, copy
> the keys in, and then do a refresh here ...
Ok, I will try that out-- I think if I do:
image-create --no-refresh
copy keys
set-publisher
That I should get a refresh on the set-publisher, right?
> client.py:
>
> - line 1665: no need to pass in img_dir if you're passing img, right?
I wasn't sure I understood the nuance of "mydir" in the caller, and
was worried about creating extraneous change. Do you want me to chase it?
> - line 1794, 1795: do you need abspath() here? Do you really expect
> img.root not to be absolute? And probably should use os.path.join()
> here, too.
os.path.join is the wrong choice here, because of it's subtle semantics.
If ssl_cert is: /var/pkg/ssl and img_root is: /foo/bar, then
os.path.join(img_root, ssl_cert) --> "/var/pkg/ssl". You can read all
about it in the doc for os.path.join.
Thankfully Bart pointed this out to me.
> imageconfig.py:
>
> - line 156: So this isn't the case in general -- it's specific to zones,
> and hopefully we'll make that difference go away in the next release.
Yes.
> - line 281: why str()?
Dunno, it was there. I think it's because this could be None, so
str'ing it gets you to "None".
> - line 471, 483: no need for backslashes.
Ok, thanks.
-dp
--
Daniel Price, Solaris Kernel Engineering http://blogs.sun.com/dp
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
