A new full webrev has been posted under http://cr.opensolaris.org/~stevep/6351623 (6351623 Initial manifest-import is slow)
This webrev includes all changes for the feedback I have received. All necessary tests had been run succesfully including the 'svcadm _smf_repository_switch' stress test that Dan suggested on the global/non-global zone environment. Please take a look at and send me any comment. If this round of code review runs smoothly then the plan is to putback this performance improvement into build 84. TIA - Steve Steve Peng wrote: > 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 >> >> > > _______________________________________________ > smf-discuss mailing list > smf-discuss at opensolaris.org >
