On 05/ 3/11 04:56 PM, Edward Pilatowicz wrote:
On Tue, May 03, 2011 at 01:28:06PM -0700, Brock Pytlik wrote:
Webrev:
http://cr.opensolaris.org/~bpytlik/ips-18240-v1/
Bug:
18240 zone proxy needed
looks much better, but i still have more makefile comments.
ed
---------
src/brand/attach:
src/brand/p2v
- please check the return code when doing:
$PKG set-property use-system-repo true
Ok.
---------
src/brand/attach:
- please leave all the --no-refresh options to the subcommands and add a
$PKG refresh --full
after we enable the system repo.
Ok. This can't be done until use-sys-repo has been set. So I've had it
do the refresh just after that property is set.
---------
src/pkg/manifests/package%252Fsysrepo.p5m
- the zone proxy should probably be delivered in it's own package since
by default we don't need a sysrepo (and apache) installed in
non-global zones. the zone proxy package would then have a global
zone only dependancy on the sysrepo package.
That doesn't seem right to me. I'm happy to add more variant tags to
actions if you'd like, but I believe this all belongs in one package.
---------
src/svc/zoneproxyd.xml
- why are zones dependant upon this service instead of the other way
around?
Don't know, I'll ask K.
- don't run as root. instead, run as daemon and add in just the privs
you need. (which i'm guessing are file_owner and file_dac_read. if
you need additional privs you can figure out which ones you need via
ppriv -D.)
I've asked K about this, if he (or you) can tell me how to do this I
will. This has not changed since the initial review months ago.
---------
src/svc/zoneproxy-client.xml
- don't run as root. instead, run as daemon and add in just the privs
you need. (which i'm guessing are net_privaddr.)
See above.
[snip]
---------
src/zoneproxy/Makefile.constants
- in general when creating makefiles that will be included in other
makefiles you don't want to mix variables definitions and dependancy
rules. there are multiple reason for this that i can explain in
person. i'd recommnd breaking this file up into Makefile.master and
Makefile.targ (with variables and rules in each file respectively).
Then in each of the subdirectory makfiles that include this makefile,
include Makefile.master at the top and Makefile.targ at the bottom.
Is this really necessary before putback? Is something being built
incorrectly? Are my binaries being produced with the wrong options? If
not, I'd just as soon leave this alone.
- using -D_TS_ERRNO is kinda weird. why not just use -D_REENTRANT?
Sure
- add some common variables here for doing INSTALLs
FILEMODE= 0644
DIRMODE= 0755
INS.file= $(RM) -f $@; $(INS) -s -m $(FILEMODE) -f $(@D) $<
INS.dir= $(INS) -s -d -m $(DIRMODE) $@
I think doing it the way it is preferable for now.
- the $(ZONES_PROG) rule shouldn't make the target directory. instead
do this via a generic dependency rule like:
$(ZONES_LIBDIR):
$(INS.dir)
$(ZONES_LIBDIR)/%: % $(ZONES_LIBDIR)
$(INS.file)
See above.
- the $(ZONES_PROG) rule shouldn't update the $(PROG) binary. instead
the $(PROG) binary should be the same as the binary installed in the
proto area. so we should run mcs on the objects when building them.
to do that i'd recommend overriding the default build rules in your
Makefile.master by doing:
.c:
$(LINK.c) -o $@ $< $(LDLIBS)
$(MCS) -d $@
.c.o:
$(COMPILE.c) $(OUTPUT_OPTION) $<
$(MCS) -d $@
Huh, I thought this was exactly what you've been telling me not to do.
But hey, ok, I'll do it this way now.
- you should put common rules for installing header files in here
instead of in zoneproxyd/Makefile. for example:
ROOTHDRDIR= $(ROOT)/usr/include
ROOTHDRS= $(HDRS:%=$(ROOTHDRDIR)/%)
$(ROOTHDRDIR):
$(INS.dir)
$(ROOTHDRDIR)/%: % $(ROOTHDRDIR)
$(INS.file)
I'd rather just leave things as they are.
- you can put common rules in here for clean and clobber:
CLEANFILES= $(PROG) $(OBJS)
CLOBBERFILES= $(ZONES_PROG) $(ROOTHDRS)
clean:
-$(RM) $(CLEANFILES)
clobber: clean
-$(RM) $(CLOBBERFILES)
Fine.
- you can put common rules in here for install_h (and remove it from
zoneproxyd/Makefile):
install_h: $(ROOTHDRS)
This is working as is, so I'm going to leave it.
---------
src/zoneproxy/zoneproxyd/Makefile
- you should be able to delete HDR_MARKER.
Fine.
---------
src/zoneproxy/list/Makefile
- you should be able to delete HDR_MARKER.
- i'd recommend just moving these list* files back into the zoneproxyd
directory.
I like them where they are, and was told to move them out, so I'm just
going to leave them there since it's actually working.
Brock
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
