David, Ok I will go ahead assert in backend_switch_copy(). I will send out another increment webrev once it is ready.
Thanks Steve David Bustos wrote: > Quoth Steve Peng on Thu, Feb 14, 2008 at 10:58:14AM -0700: > >> David Bustos wrote: >> >>> backend_switch() no longer fails with _TRUNCATED, so please remove it >>> from repository_switch()'s comment. >>> >> Line 1172 in backend_switch_copy possibly can return _TRUNCATED error >> hence backend_switch can return the same >> error. >> > > Not if you assert() that backend_switch_copy() doesn't fail with > _TRUNCATED. If it does, we'll abort and the caller of backend_switch() > will never see the error. > > >>>>> cmd/svc/configd/backend.c >>>>> >>> ... >>> >>>>> 1302: Shouldn't you assert() that this doesn't fail with _TRUNCATED >>>>> since you know that dst is much shorter than PATH_MAX? >>>>> >>>>> >>>> I can add that. >>>> >>> Then you should also fix backend_switch()'s comment to say that it can >>> return the error codes from backend_switch_copy() except for _TRUNCATED. >>> Though I think it would be better to just list the errors outright. >>> >> Look into more I think I should back out assertion since strcat in >> backend_switch_copy can possibly fail with _TRUNCATE even >> it may not happen but I like to be more cautious. >> > > No, we can prove that it can't happen. It doesn't depend on the value > of sw_back, it depends on REPOSITORY_DB, FAST_REPOSITORY_DB, PATH_MAX, > and the code of backend_switch_copy(), all of which are either standard > or are compiled into the binary. It means that somebody changed one of > those things in an uncorrelated way and is testing it. Rather than > passing an error up the stack which eventually results in a strange > error message, you should abort() as soon as we know it's a programming > error. (Some would even argue that you could assert() when strlcat() > fails in backend_switch_copy().) This will make debugging easier and > eliminates an error case for the calling code. > > If you want to make the assumptions more explicit, you could calculate > how long dst has to be before backend_switch_copy() returns _TRUNCATED > and document it in the comment. Then you could add > assert(strlen(dst) < /* computed number */) to backend_switch(). And > you could even add a comment to the definition of REPOSITORY_DB and > FAST_REPOSITORY_DB saying that it shouldn't be made any longer than your > computed number. I don't think it's worth it for such specialized code > in the same file, though. > > > David >
