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
---------
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.
---------
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.
---------
src/svc/zoneproxyd.xml
- why are zones dependant upon this service instead of the other way
around?
- 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.)
---------
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.)
---------
src/zoneproxy/zoneproxy-adm/Makefile
src/zoneproxy/zoneproxy-client/Makefile
- this seems unnecessary since the common makefile does it:
CPPFLAGS += -I../zoneproxyd
---------
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.
- using -D_TS_ERRNO is kinda weird. why not just use -D_REENTRANT?
- 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) $@
- 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)
- 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 $@
- 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)
- 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)
- you can put common rules in here for install_h (and remove it from
zoneproxyd/Makefile):
install_h: $(ROOTHDRS)
---------
src/zoneproxy/zoneproxyd/Makefile
- you should be able to delete HDR_MARKER.
---------
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.
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
