[http://cr.opensolaris.org/~mwaterl/6739234.changes_only/webrev/]
Moriah Waterland writes:
> > usr/src/lib/Makefile
> >
> > 96,98: this doesn't look like the right place for these things. The
> > libraries after the .WAIT on line 102 do _not_ depend implicitly on
> > these two libraries.
> Done. I moved libinstzones and libpkg to after the last .WAIT under
> SUBDIRS.
OK; that sounds right.
> However, I am a bit puzzled about the locations of those .WAITs under
> SUBDIRS. It seems improbable that everything after each of the .WAITs
> actually depends on the libraries before them. Am I missing something?
There's a block comment that explains things -- lines 33-43.
> > 75: nit: format string should be const, and should have /*
> > PRINTFLIKE1 */ here.
> Done, only sort of. I added the /* PRINTFLIKE1 */ but had problems with
> changing the format string. Please see Note 7.
The cascade errors are due to the definitions of functions like echo()
and echoDebug() in libinst.
> > usr/src/lib/libinstzones/common/instzones_lib.h
> > 305,307,309,344,363,366: fmt should be const.
> Left alone. Tried this change, but ran into build problems in
> usr/src/cmd/svr4pkg. I had made the changes that you suggested as well
> as adding const to the function definitions for:
> o _z_add_arg()
> o _z_program_error
> o _z_strPrintf_r
> o _z_strPrintf
> o _z_echo
> o _z_echoDebug
> o _z_program_error()
> Please see Note 7. for more information about build issues. I would be
> happy to file a bug on this if you like. Please just let me know.
Please do file a bug.
> > usr/src/lib/libinstzones/common/zones_utils.c
> >
> > 125,127: why are errors printed to stdout instead of stderr?
> Fixed. Please see Note 8 regarding the origin of this code. I changed
> the printf(s) to
> o (void) fprintf(stderr, "Allocation of memory failed\n");
> and
> o (void) fprintf(stderr, "ERROR: code %d\n", error_num);
> Also, I changed the declaration of "int errno" to be "int error_num" to
> avoid confusion with the errno global variable.
Both changes sound good to me.
> > usr/src/lib/libpkg/common/p12lib.c
> > usr/src/lib/libpkg/common/p12lib.h
> > Please seek out legal help with this part. I don't think that
> > putting this code (which clearly belongs to someone else) under CDDL
> > is the right thing to do. The original Sun copyright notice
> > (without CDDL) was probably correct.
> >
> > Plus, you'll need to update the third party license files.
> Removed CDDL from both p12lib.c and p12lib.h. In usr/src/lib/libpkg, I
> created a THIRDPARTYLICENSE file and included the OpenSSL copyright and
> license as well as the AT&T copyright. I also created the
> THIRDPARTYLICENSE.descrip file. I modified the package definition for
> SUNWpkgcmdsu to include these licenses.
OK. As long as you've had the necessary legal reviews and you've
followed the instructions in $SRC/README.license-files, I'm happy.
> > usr/src/lib/libpkg/common/pkgweb.c
> >
> > 1831-1832: what is this?
> Removed comments. But left the code changes there. The changes were
> required to address the problems from tmpfile[] array being declared as
> static, but was then redefined multiple times elsewhere in the file.
>
> + /ws/onnv-tools/SUNWspro/SS12/bin/cc
> .
> .
> "../common/pkgweb.c", line 1832: warning: name redefined by pragma
> redefine_extname declared static: tmpfile (E_PRAGMA_REDEFINE_STATIC)
> cc: acomp failed for ../common/pkgweb.c
> *** Error code 2
The reason the compiler was complaining is that "tmpfile" is the name
of a standard library function. The name of the variable itself is
bad, not the storage specifier. Removing "static" doesn't fix this
problem properly.
In fact, removing "static" creates a new problem. The code is now
broken because of line 1886: this array is returned to the caller. It
*cannot* be automatic storage. It must remain static.
> > usr/src/lib/libpkg/common/progerr.c
[...]
> > 48: errors on stdout doesn't seem right.
> Fixed. Changed to:
> 45c45
> < error_and_exit(int errno)
> ---
> > error_and_exit(int error_num)
> 47c47
> < (void) printf("%s\n", strerror(errno));
> ---
> > (void) fprintf(stderr, "%d\n", error_num);
OK. It's a shame there's no good way to print the error values passed
into error_and_exit(), but this seems better than the random values
that were there before.
> > usr/src/pkgdefs/SUNWpkgcmdsr/prototype_com
> >
> > 34-36: assignments shouldn't be needed here.
> Left alone. I think that they are necessary. I tried removing these
> assignments, but then I saw the following errors:
> opensores# make pkg
> pkgmk -f prototype_i386 -d
> /export/ws/mwaterl/stepchild_of_master/packages/i386/nightly-nd \
> -r "/export/ws/mwaterl/stepchild_of_master/proto/root_i386 " -o
> SUNWpkgcmdsr
> ## Building pkgmap from package prototype file.
> pkgmk: ERROR: unable to open pkginfo file <0&>: No such file or
> directory
> pkgmk: ERROR: Invalid combinations of zone parameters in pkginfo file
> ## Packaging was not successful.
> *** Error code 1
> make: Fatal error: Command failed for target `pkg'
I'm not willing to sign off on that one just yet. This requires some
deeper investigation. Those assignments make no sense, and I'm not
sure that the package is being produced properly even when it shows no
errors.
(It concerns me because we have IPS conversion down the road
somewhere, and "weird" packages will likely make that job harder.)
> > usr/src/pkgdefs/SUNWpkgcmdsu/prototype_com
[...]
> > 80: we typically don't ship compilation symlinks for internal
> > libraries. Will anything outside of ON compile against this?
> Eventually no, nothing outside of ON should compile against this. But,
> it is currently being used by projects in the new OpenSolaris installer:
> o http://src.opensolaris.org/source/xref/caiman/slim_source/
Oh. You might consider using a SUNWpkgcmdsint (internal) package to
deliver consolidation private objects.
> Note 7.
> =======
>
> I ran into the following problem when I added const to the fmt strings
> in libinstzones:
As above; it's complaining about the libinst functions. There are
only a handful of these.
> Note 8.
> ======
> These files were updated in 2008 out in the project gate for the new
> OpenSolaris installer. I pulled them directly from that gate and
> forgot to update the copyright date.
OK.
--
James Carlson, Solaris Networking <james.d.carlson at sun.com>
Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
