David, Responses inline.
Thanks Steve David Bustos wrote: > 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? > Will do. > 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. > Will be removed once the ability of specifying src and dst are turned off. > 1781 (_TRUNCATED): Shouldn't this description say that rpr->rpr_dst is > too long? > Ditto. > 1785: backend_switch() can also fail with _BACKEND_ACCESS and > _NO_RESOURCES. Please document them here. > Done. > cmd/svc/configd/backend.c > 768,783,795: You need to free(buf) before you return. > Had been changed right after I sent out the code review request. Considered done. > 850: Can this be wrapped onto the previous line? > Done. > 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. > Ok. > 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. > Sounds good. Comments were added. > 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? > Yes, it can return _BACKEND_READONLY error if writing flag is set. The assertion here is from the previous recommendation. Changed so it now can fail with this error. > 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. > I won't hesitate to take this approach. :-) Actually I think both src and dst should be backend private (client shouldn't know about) and dst is determined by client provided switch flag (direction). > 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. > Good point. I will look into this more. > 1343: "integrity check" means something different in SQLite, so please > change this to something like "sanity check". > Done. > 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"? > May no longer be an issue. See the previous response wrt be_path. > 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? > Changed. > 6867: Can't this also fail with the errors that repository_switch() in > client.c can fail with? Namely _PERMISSION_DENIED, > _BACKEND_READONLY, > Changed. > 6872: struct rep_protocol_switch_request is pretty big. Can you > malloc() it so we won't segfault if we're out of memory? > My not be an issue once we reject client's ability of specifying src and dst. > 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. > Sounds good. I will file a bug later. > 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. > abort works with me. > cmd/svc/milestone/manifest-import > 470: Please indent this by half a tab. > Done. > David >
