Antonello, A few more responses in line. I think that we're all in agreement now. Sean, Tony and I are working on coding the changes.
tom Antonello Cruz writes: > On 02/19/10 02:55 PM, Tom Whitten wrote: ... > > Antonello Cruz writes: > ... ... > > > >> > >> 953: Isn't setemilog always true here? > > > > setemilog is set to signal whether to configure method log. Would you prefer > > > > int setemilog; > > .. > > .. > > setemilog = emi_set_state(RESTARTER_STATE_OFFLINE, B_TRUE); > > > > which is simpler but doesn't show that we're using 'setemilog' to > > determine whether to set the log? > I think the code would be less ambiguous that way. > I was confused because I thought I had missed where setemilog could have > changed its value since its initialization. You don't need to change the > code, but maybe a comment could be of some help here... > If you keep the code as it is, would you consider initializing setemilog > to B_TRUE instead of one? We'll change the code as proposed to make it less ambiguous. > > ... > >> > >> usr/src/cmd/svc/svccfg/svccfg_engine.c > >> 736: why we return 1 if there were no manifests to import? Is that > >> an error? from svccfg.y only -2 generates error. > > > > This falls into the category of preserving the behavior of the existing > > code. Take a look at lines 575-582 of the old code. mhash_test_file() is > > called to check the hashes for the manifest file. If the hashes are OK and > > there is no work to be done, mhash_test_file() returns MHASH_RECONCILED and > > this value is returned to the caller of engine_import(). MHASH_RECONCILED > > == 1, so the EMI code also returns 1 if there is no work to be done. It is > > not an error if there is no work to do. > > > > That's the history. As you point out, though, nobody looks to see if 1 is > > returned, so we could just as easily return 0 to indicate there is no > > error and be more consistent with the rest of the function. > Or you can put a comment on the top of engine_import() stating that the > return value of 1 indicates there is nothing to import. You don't have > to change anything, I just wanted to be sure that a return value was not > being overlooked That sounds like a good solution. We'll do it. > > > > >> > >> 757: What is the case where you get manifests == NULL here? > >> Shouldn't this be an assert()? > > > > It happens if find_manifests() is called on line 716 with a directory that > > contains no manifests that need to be imported. See the comments for the > > find_manifests() function. An assert would not be appropriate. > I was somehow under the impression that if find_manifests() was called > on an empty dir it would return -1 and not 0. Please make sure you test > this case. > Will do. It actually gets tested every time we reboot a system with no changed manifests.
