<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 TRANSITIONAL//EN"> <HTML> <HEAD> <META HTTP-EQUIV="Content-Type" CONTENT="text/html; CHARSET=UTF-8"> <META NAME="GENERATOR" CONTENT="GtkHTML/3.32.2"> </HEAD> <BODY> On Thu, 2011-09-15 at 22:22 -0500, Jamey Sharp wrote: <BLOCKQUOTE TYPE=CITE> <PRE> On Thu, Sep 15, 2011 at 07:28:25PM -0400, Gaetan Nadon wrote: > On Wed, 2011-09-14 at 10:07 -0500, Jamey Sharp wrote: > > "configure --with-int10=yes" is not a valid configuration, and the check > > It depends what is the definition of "valid" in this context. Running > "./configure --with-int10" will set "INT10" to "yes". You may choose to > ignore this value. I can only guess that current code checked for "yes" > as a means to provide a default value when no backend was specified. On further investigation: Daniel Stone introduced the block I'm proposing to delete in commit 588105173840355717d7b2f7f652289a41166c3f from 2005, with a commit message beginning "Huge cleanup." It appears that he intended that patch to mostly be a reorganization of configure.ac, not to change functionality, so I'm not sure how this snuck in. Otherwise, as far as I can tell, there's never been an attempt to support INT10=yes. </PRE> </BLOCKQUOTE> That was my guess as well. <BLOCKQUOTE TYPE=CITE> <PRE> > > for sys/vm86.h and sys/io.h is not used. Delete it. > > I agree with ignoring "yes". I don't recall any other module using it in > that way. Just be prepared in the case where someone did use it. It's > not really dead code, but close enough. > > The default value is either x86emu or stub for FreeBSD on a PowerPC. Any > unrecognized value (such as yes, no or vn86) will not build any int10 > backend. No warnings or errors. Hopefully the builder will have some way > of finding out why it does not work. The library builds with just the > common code. "vn86" is an excellent example. This does seem error-prone. But: > Suggestion: > > AS_HELP_STRING([--with-int10=BACKEND], [vm86, x86emu or stub (default:auto)]), > > AC_MSG_ERROR if no valid value is given Can you suggest a patch that does that? I don't see any way to report such an error without copying the list of possible backends yet another time. </PRE> </BLOCKQUOTE> The vast majority of configure option do not make "user input validation". If you think this option does not represent an above average danger for the user, I am fine. Keep in mind I don't know the server functionality :-) <BLOCKQUOTE TYPE=CITE> <PRE> > Verify if there is a need to check for the headers. The code as > it is was probably the result of changes around it rather than > the intention of the developer. Not as far as I can tell; and since nobody could have gotten a usable int10 module with --with-int10, and the AC_CHECK_HEADERS is only used in that case, I don't think anybody is relying on it. So is there some reason not to apply this patch as-is? </PRE> </BLOCKQUOTE> Nope, but it would be nice to improve the help text in AS_HELP_STRING... No need to resubmit just for that text change.<BR> <BR> Reviewed-by: Gaetan Nadon <memsize@videotron.ca> <BLOCKQUOTE TYPE=CITE> <PRE> Thanks for looking this over! Jamey </PRE> </BLOCKQUOTE> <BR> </BODY> </HTML>