Tom Whitten wrote: > Keep in mind that assert() can be compiled out if NDEBUG is defined. I > don't think that this is done very often, however. > > I've seen some places in the code where there is an assert() followed by an > abort(). > Yes, that will be the case here. - Steve
> tom > > Steve Peng writes: > >> 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 >>
