David, Updated incremental webrev is ready and is under the same location:
http://cr.opensolaris.org/~stevep/6351623-inc/ Steve Steve Peng wrote: > 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 >> >> > > _______________________________________________ > smf-discuss mailing list > smf-discuss at opensolaris.org >
