Quoth Steve Peng on Mon, Jan 28, 2008 at 04:02:45PM -0700:
> well. I have posted the new incremental webrev under
>
> http://cr.opensolaris.org/~stevep/6351623-2
- In the future, could you please also produce a full webrev?
cmd/svc/configd/client.c
1780: _BAD_REQUEST can also happen if the src or dst buffers are
unterminated. Please note this in the comment.
1781 (_TRUNCATED): Shouldn't this description say that rpr->rpr_dst is
too long?
1785: backend_switch() can also fail with _BACKEND_ACCESS and
_NO_RESOURCES. Please document them here.
cmd/svc/configd/backend.c
768,783,795: You need to free(buf) before you return.
850: Can this be wrapped onto the previous line?
861: I would add a comment explaining that the format string is broken
up to prevent expansion by SCCS, but you don't have to.
1113: Since backend_create_backup_locked() can now fail with
_NO_RESOURCES, you have to add it to backend_create_backup()'s
comment, and to backup_repository()'s comment in client.c. And you
should add a case to _scf_request_backup()'s switch in svcadm.c, but
that's not strictly necessary.
1267: backend_lock()'s comment says it can fail with _BACKEND_READONLY
if writing is nonzero. How do you know that won't happen here?
1272: What will happen if src is different from be->be_path?
I suppose src will probably be differently sized, so
backend_switch_copy() will fail. If they happen to be the same
size, but with different contents, then any data we have cached in
memory could be wrong. I don't know what would happen, but I don't
think I can rule out it being very bad. I think it would be better
to just use be->be_path and stop allowing src to be specified by the
client.
1275: Let me see if I understand correctly the case where result !=
REP_PROTOCOL_SUCCESS but sw_back is nonzero. We were operating on
one repository, which we probably modified, but we were unable to
copy it to the destination, and you want to switch to the
destination repository anyway? If we modified the source
repository, then couldn't we have some of that data cached in
memory? If so, then won't that data be incorrect when we switch to
the different destination repository? If you want to do this,
I think you need to flush the cache. I suspect it will be
sufficiently rare that we can do that later, and for now you can
just fail into maintenance mode.
1343: "integrity check" means something different in SQLite, so please
change this to something like "sanity check".
lib/libscf/common/lowlevel.c
6863: proto_error() translates _BAD_REQUEST into _INVALID_ARGUMENT, so
shouldn't you also say that _INVALID_ARGUMENT can happen if "backend
path does not match"?
6866: proto_error() translates _TRUNCATED and _UNKNOWN into _INTERNAL,
so shouldn't you say that _INTERNAL can happen if
repository_switch() encounters a strlcpy error or a file operation
error?
6867: Can't this also fail with the errors that repository_switch() in
client.c can fail with? Namely _PERMISSION_DENIED,
_BACKEND_READONLY,
6872: struct rep_protocol_switch_request is pretty big. Can you
malloc() it so we won't segfault if we're out of memory?
cmd/svc/svcadm/svcadm.c
2405,16: If these happen, the exit(1) will make them difficult to
debug. I think you should print an explanation. I suppose you
could file a bug for that, though.
2447: I think you should abort() if we receive an unknown error, since
we should be able to know what errors the function can produce. But
if you're not going to do that, I recommend somehow working
scf_error() or scf_strerror() in, so we can tell which error code
was returned.
cmd/svc/milestone/manifest-import
470: Please indent this by half a tab.
David
