Jim,
Here is the rest of my responses to your code review comments. I will
be generating a new webrev and will send that out to my code reviewers
later this week.
thanks,
Moriah
James Carlson wrote:
> 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. I also alphabetized the rest of the entries after that .WAIT.
o was: SUBDIRS += \
.
libinstzones \
.
libpkg \
.
krb5 .WAIT \
libsmbfs \
libfcoe \
libstmf \
libnsctl \
libunistat \
libdscfg \
librdc
o now: SUBDIRS += \
.
krb5 .WAIT \
libdscfg \
libfcoe \
libinstzones \
libnsctl \
libpkg \
librdc \
libsmbfs \
libstmf \
libunistat
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?
> usr/src/lib/libinstzones/Makefile.com
>
> 58: this should probably be a lazy load for libzonecfg.
Done. I enclosed load for libzonecfg:
o LDLIBS += -lc -lcontract $(ZLAZYLOAD) -lzonecfg $(ZNOLAZYLOAD)
I also changed libzonecfg in usr/src/cmd/svr4pkg/pkgcond/Makefile.
> usr/src/lib/libinstzones/common/instzones_api.h
>
> 62-69: suggest removing comment blocks that you're not actively
> using.
Done
> 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.
> usr/src/lib/libinstzones/common/instzones_lib.h
>
> 62-65: delete
Done, and I combined next two comments (67 - 74).
> 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.
> 377: use ANSI '(void)'.
Done
> usr/src/lib/libinstzones/common/llib-linstzones
>
> It's unclear to me what the distinction is between instzones_api.h
> and instzones_lib.h. I thought instzones_lib.h was something
> internal to the construction of the library, and not used by things
> that link to the library. If that's correct, then it should *NOT*
> be included here. Only things that consumers of the library use
> belong in the llib-l* file. If instzones_lib.h is actually used by
> the consumers, then maybe the API is ill-defined.
Removed instzones_lib.h and verified that it was not used by anything
outside of library.
> 33: this is almost certainly not right. There's nothing here for
> lint to consume.
Removed.
> usr/src/lib/libinstzones/common/mapfile-vers
>
> Missing "MAPFILE HEADER" block. Should cause 'nightly' build
> errors.
Fixed.
> 25: stray
Removed this.
> usr/src/lib/libinstzones/common/zones.c
>
> 23: lost a year? (Several files here are similar.)
Fixed, please see Note 8.
> 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.
> 125: how does "%s" help here? (Looks like a concern about
> translation attacks that I think were fixed long ago, but not sure
> ...)
Yea, umm...please see Note 8.
> 639,653,665,677: nit: correspondence between comments and code is a
> little arbitrary.
Left alone, please see Note 8.
> usr/src/lib/libinstzones/i386/Makefile
>
> 28: missing $(ROOTLINT)? (Have you built nightly on x86?)
Fixed, and yes. I haven't actually been using the lint libraries
though, because the pkgcmds are not lint free.
> usr/src/lib/libpkg/Makefile.com
>
> 55: stray
Removed this.
> usr/src/lib/libpkg/common/ckparam.c
>
> 31: as long as you're here, this line is wrong and needs to be
> removed.
Removed this.
> usr/src/lib/libpkg/common/ckvolseq.c
>
> 31: ditto
Removed this.
> 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.
> usr/src/lib/libpkg/common/pkglib.h
>
> 614: as long as you're adding a function, giving it a proper
> prototype would be nice.
Fixed the function definition for getmapmode as well as this
declaration. Changed usr/src/lib/libpkg/common/gpkgmap.c as follows:
122c122
< getmapmode()
---
> getmapmode(void)
> 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
> usr/src/lib/libpkg/common/progerr.c
>
> 31: misplaced.
Removed this.
> 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);
> usr/src/lib/libpkg/i386/Makefile
>
> 28: stray
Removed.
> usr/src/pkgdefs/SUNWpkgcmdsr/depend
>
> This doesn't look like a significant 'depend' file. Is it needed?
Removed this depend file and am using the default. Additionally, in
SUNWpkgcmdsu I followed advice from mjn about how to dynamically create
a depend file from the default depend along with additional dependencies.
> 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'
> usr/src/pkgdefs/SUNWpkgcmdsu/prototype_com
>
> 33,34: stray
Left alone, please see note above.
> 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/
Note 7.
=======
I ran into the following problem when I added const to the fmt strings
in libinstzones:
main.c: In function `main':
main.c:174: warning: passing arg 1 of `z_set_output_functions' from
incompatible pointer type
main.c:174: warning: passing arg 2 of `z_set_output_functions' from
incompatible pointer type
main.c:174: warning: passing arg 3 of `z_set_output_functions' from
incompatible pointer type
+ /ws/onnv-tools/SUNWspro/SS12/bin/cc -O -xspace -Xa -xildoff
-errtags=yes -errwarn=%all -erroff=E_EMPTY_TRANSLATION_UNIT
-erroff=E_STATEMENT_NOT_REACHED -xc99=%none -W0,-xglobalstatic -v
-DTEXT_DOMAIN="SUNW_OST_OSCMD" -D_TS_ERRNO
-I/export/ws/mwaterl/stepchild_of_master/proto/root_i386/usr/include
-I/export/ws/mwaterl/stepchild_of_master/usr/src/cmd/svr4pkg/hdrs
-I/export/ws/mwaterl/stepchild_of_master/usr/src/lib/libpkg/common
-I/export/ws/mwaterl/stepchild_of_master/usr/src/lib/libinstzones/common
-D_FILE_OFFSET_BITS=64 -c main.c
"main.c", line 174: warning: argument #1 is incompatible with prototype:
prototype: pointer to function(pointer to const char, ...)
returning void :
"/export/ws/mwaterl/stepchild_of_master/usr/src/lib/libinstzones/common/instzones_api.h",
line 127
argument : pointer to function(pointer to char, ...) returning
void (E_ARG_INCOMPATIBLE_WITH_ARG)
"main.c", line 174: warning: argument #2 is incompatible with prototype:
prototype: pointer to function(pointer to const char, ...)
returning void :
"/export/ws/mwaterl/stepchild_of_master/usr/src/lib/libinstzones/common/instzones_api.h",
line 127
argument : pointer to function(pointer to char, ...) returning
void (E_ARG_INCOMPATIBLE_WITH_ARG)
"main.c", line 174: warning: argument #3 is incompatible with prototype:
prototype: pointer to function(pointer to const char, ...)
returning void :
"/export/ws/mwaterl/stepchild_of_master/usr/src/lib/libinstzones/common/instzones_api.h",
line 127
argument : pointer to function(pointer to char, ...) returning
void (E_ARG_INCOMPATIBLE_WITH_ARG)
cc: acomp failed for main.c
*** Error code 3
make: Fatal error: Command failed for target `main.o'
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.
