From: Sam S. <sd...@gn...> - 2016-08-30 15:11:59
|
> * Daniel Jour <qna...@tz...> [2016-08-28 23:55:18 +0200]: > >> 15620::::Daniel Jour 2016-06-19 regexp: use autotools, gnulib, improve >> buffer handling >> >> 0! This is 3 patches in one: > > Yes, this is too much for a single change. I was working on all three > changes at the same time, which is how this huge commit came into > being (sorry). This is an absolute NO. Each commit must reflect a single logical change. It's hard, it requires discipline and focus, but this is the only way. >> 1. Why are you replacing stack allocation with arbitrary limits? > > I don't? I'm switching the buffer size for the error message from > BUFSIZ (which is an unrelated constant that could be easily too much > for the stack) to another constant SAFE_ERROR_MESSAGE_LENGTH which we > can define to a known "good" value (128 is a first start .. if there's > an error message that's longer we can expand this, and consistently > test whether that's too much on the stack or not). BUFSIZ is a pretty standard constant for all string buffers. > The other change is more important: With a carefully crafted regex > expression one can currently crash (or potentially worse) CLISP, > because the buffer for the matches can overflow the stack. Let us revisit this issue at a later date. I think the with_string_0 mechanism is good enough. If disagree, you will have to argue for it to be changed pervasively throughout CLISP. >> 2. Is it really necessary to create a separate m4 directory for the >> single file gnulib-cache.m4? It's really ugly! (same for the >> rawsock change below). > > Yes, that's the directory where all the gnulib (m4 macros) should > reside in, too. Running gnulib-tool --update in the regexp module > directory populates that directory (as well as lib/). This is why we don't run gnulib-tool in that directory! We only ever run it in src. > I think the current state (in which I commited this) is unfortunate, > though: The gnulib files (in m4/ and lib/) should be put under (our) > revision control, too. What do you think would be the best approach > here? > (gnulib has some discussion about this very issue: > https://www.gnu.org/software/gnulib/manual/gnulib.html#VCS-Issues) The gnulib file we import are already under our VCS in clisp/src/glm4 and clisp/src/gllib. >> 15625:::tip:Daniel Jour 2016-08-23 rawsock: autotools build system, >> own gnulib checkout >> >> Copying code from socked.d into rawsock.c is no good. > > Yes, this is a (dirty) work around: I was not able to update the > gnulib code for core CLISP (thus socket.d). what was the problem? >> Also, you removed the configure script, so now people have to install >> autoconf to build clisp. >> Are you sure this is right? > > I removed it from the repository because it is a generated file > now. It would be part of a source distribution (a release) though, > thus only CLISP developers need to have autoconf/automake/etc. Okay, so you want to go the way of Emacs - the developers have to install autotools and the generated files are excluded from VCS. Fine. Let us do that after the release. Note, however, that you should use "hg mv" for configure.in --> configure.am transition and make changes to configure.am only after committing the "mv" operation (same for _all_ renaming). >> DIUC that the main change required to make rawsock work on windows >> was the switch from rawsock_t to int? > > Basically, yes. gnulib is (or in part, will be) handling the windows > specific code. rawsock is interfacing to POSIX/BSD sockets, gnulib is > implementing them on windows. Fine. This means that the change necessary for a release is actually quite small: --8<---------------cut here---------------start------------->8--- diff -r 2d1aedc0b550 modules/rawsock/rawsock.c --- a/modules/rawsock/rawsock.c Mon Aug 29 19:20:13 2016 -0400 +++ b/modules/rawsock/rawsock.c Tue Aug 30 11:00:39 2016 -0400 @@ -66,7 +66,7 @@ #if defined(HAVE_IFADDRS_H) # include <ifaddrs.h> #endif -typedef SOCKET rawsock_t; +typedef int rawsock_t; DEFMODULE(rawsock,"RAWSOCK") --8<---------------cut here---------------end--------------->8--- Right? >> please take a look at clisp/modules/berkeley-db/bdb.c:bdb_handle() >> for getting the regex_t pointer out of the struct. > > Hm .. is this faster than the approach that I took? It saves a level > of indirection ("one pointer"), right? Or is there something else > wrong with the approach I took in regexp? It's the same but it allows restarts (i.e., the user can supply a different input if original argument is bad - instead of aborting the whole thing). In fact, you might want to model your approach on what I did in modules/pcre instead of modules/berkeley-db. However, this should be done _after_ the release. PLAN: -1- fix rawsock on windows and make a release (2.50) 2/3 *.d --> *.c rename 2/3 switch to autotools (dropping generated files) -4- update gnulib -5- your proposed regexp changes -6- release (3.0) Did I miss anything? -- Sam Steingold (http://sds.podval.org/) on darwin Ns 10.3.1404 http://www.childpsy.net/ http://think-israel.org http://camera.org http://truepeace.org http://www.memritv.org http://dhimmi.org Press any key to continue or any other key to quit. |