Quoth Steve Peng on Tue, Jan 22, 2008 at 11:38:24AM -0700: > Thanks for your comments. Replies inline. With regard to idempotency, > I think the protocol should be idempotent and handle accordingly.
Does that mean you'll change client.c:repository_switch() to use changeid logic? > David Bustos wrote: > > Quoth Steve Peng on Tue, Jan 15, 2008 at 08:16:16PM -0700: > >> David Bustos wrote: ... > >> > 1256: Why is it ok to fail if src doesn't match? What should the > >> > client do in that case? > >> > >> Good point. I probably should assert it. > > > > No, then a bug in svcadm could cause svc.configd to crash. What > > I wonder is if src is really necessary; shouldn't we always switch from > > the current repository? If so, why should we refuse if the client has > > the wrong path? > > What bug can cause configd to crash? If you make svc.configd assert() that strcmp(be->be_path, src) == 0, then if svcadm had a bug where it gave the wrong path to _scf_repository_switch(), then the condition would be false and svc.configd would abort. > Shouldn't the path (be_path) > always match the existing be_db? Is there any > possibility that be_path can refer to a file path which is different > from what be_db points to? I'm talking about line 1254, where you compare be->be_path to src, the path the client sent. The only guarantee that they will match is that svcadm knows where svc.configd's repository is. This seems like an unnecessary restriction to me, and would make the system more fragile to bugs or future changes. Stepping back, I don't understand how the system could change so that it would make sense to request svc.configd to move its repository from one it's not using to somewhere else. It seems to me that the client should at most tell svc.configd where to move to, and svc.configd should always switch from the repository it's currently using. The only choice in source that I see is when svc.configd is using multiple database files, which is the case today because it uses persistent and nonpersistent files. But even then I would think that which to switch should be selected abstractly (i.e., persistent vs. nonpersistent), rather than by sending a path string. > >> > 1273: If backend_switch_copy() failed, but sw_back is true, will we > >> > try to sqlite_open(dst) anyway? > >> > >> Are you refering to line 1278? Do you see any reason that we shouldn't > >> sqlite_open? > > > > No, I was just confused from the comment. I do wonder, though, if we > > really should switch to the file the client gave us, or if we should > > switch back to the file which we opened when whe first started. > > In this case, I prefer switching to the original /etc/svc repository > instead of staying in the tmpfs location. Right, I agree. I'm just wondering if we should try to open whatever path the client gave us, or if we should ignore dst and always open REPOSITORY_DB. > >> > lib/libscf/inc/libscf_priv.h > >> > 297-301: Why do we need a structure? Can't _scf_repository_switch() > >> > accept sw_src, sw_dst, and sw_back as separate parameters? > >> > >> Yes, I was thinking that too but decided to put them in a structure so > >> in the future if we decide to add more parameters we can do so w/o chaning > >> the interfaces prototype. > > > > Do you know of other places in ON where we have done that? > > I think it should be easy to find the code where the interface/function > can be expanded easily by adding new fields in the > structure passed to the function. Say in the near future, if we decide > to pass more than three parameters we can do so by > just adding new members to the structure w/o changing the interface > prototype. This can be useful if the interface is called > everywhere. Right, this would be a good idea if we add fields which we don't need to modify current consumers for. Since this is a private function, I don't think that there will be enough consumers to justify the extra structure. But I'll leave it up to you. David
