Hi Thierry, remade patch for support of number other than integer in hlua_socket_settimeout() ...from latest source so should be able to be applied.
Desc: instead of accepting only integers, allow user to specify float and double when hlua_socket_settimeout() is called. Round up user specified value and validate against maximum integer to prevent overflow. Mark Lakes Signal Sciences | www.signalsciences.com | On Thu, Mar 8, 2018 at 1:24 AM, Thierry Fournier <[email protected]> wrote: > Hi Mark, > > Thanks, it seems perfect. But, I can’t apply on current master branch, the > patch is rejected. > > I forgot 3 things while my first read: > > - The Lua error are not trigerred with a return 1 (the return 1 is a bug > in the original function), but with something like that: > > WILL_LJMP(luaL_error(L, "settimeout: cannot set negatives values")); > > The return became useless because the luaL_error() function never > returns. > (it soes a longjmp call). > > - I think that timeouts < 1s are allowed. The cli refuse these ones, > but the HAProxy core is ready to work with timeout less than 1s. Note > that, When you remove this check, negative timeouts could be accepted > (the check < 1000 chek also for negative values) and this is a bug. > > - The doc (doc/lua-api/index.rst) should be updated (actuelly, line 1899: > .. js:function:: Socket.settimeout(socket, value [, mode])). It will be > necessary to precise that the fucntion accept Number in place of Integer > and, number with fractional part are allowed. > > I join two of my fixes. The third patch is yours, check if you agree with > the content and the commit message. > > BR, > Thierry > > > > > > > > > On 8 Mar 2018, at 01:17, Mark Lakes <[email protected]> wrote: > > > > Hi Thierry, thanks for feedback. Addressed concerns in the new attached > patch. > > > > http://w3.impa.br/~diego/software/luasocket/tcp.html#settimeout > > > > Description: instead of hlua_socket_settimeout() accepting only > integers, allow user to specify float and > > double as well. Convert to milliseconds much like cli_parse_set_timeout > but also sanity check the value. > > > > -mark > > > > > > On Wed, Mar 7, 2018 at 9:55 AM, Thierry Fournier <[email protected]> > wrote: > > Hi Mark, > > > > Thanks for the patch. I don’t like usage of floating point, but the > > luasocket documentation says that the settimeout() function accept only > > second. In this case, the usage of floating point seems be to be a good > > way. > > > > Can you split in a second commit the fix of comments from the effective > > patch, and avoid this kind of changes: > > > > - int tmout; > > + int tmout; > > > > Just because, this kind of changes are useless, and it add noisy > > information in the patch. > > > > A last point: could you explain int the message of the patch the > > goal of these patch. To avoid a search, this is the link of the official > > luasocket setimeout function: > > > > http://w3.impa.br/~diego/software/luasocket/tcp.html#settimeout > > > > Thanks > > Thierry > > > > > > > On 7 Mar 2018, at 18:16, Mark Lakes <[email protected]> wrote: > > > > > > In regards to earlier conversation, herein is a patch attached for the > feature. > > > From the mail archive: > > > https://www.mail-archive.com/[email protected]/msg27806.html > > > https://www.mail-archive.com/[email protected]/msg27807.html > > > > > > Mark Lakes > > > Signal Sciences | www.signalsciences.com | > > > > > > conversation participants: > > > Willy Tarreau > > > Adis Nezirovic > > > Nick Galbreath > > > > > > --------- Last conversation and decision agreement ---------- > > > Nick Galbreath Thu, 09 Nov 2017 20:44:28 -0800 > > > > > > thanks wily. > > > > > > re: " CONTRIBUTING in the sources directory," - > > > > > > yes, that is what I was looking for! thanks for the tip. > > > > > > re: least it seems important to round up non-null values to the next > > > millisecond. > > > > > > Definitely, we can and should add some checks for invalid values, etc. > > > > > > I'll read CONTRIBUTING, and set up my dev env, try a patch, and report > > > back appropriately. > > > > > > regards, > > > > > > n > > > > > > On Thu, Nov 9, 2017 at 8:37 PM, Willy Tarreau <[email protected] > > > > wrote: > > > > > > > Hi Nick, > > > > > > > > On Thu, Nov 09, 2017 at 08:27:29PM -0800, Nick Galbreath wrote: > > > > > Hello Adis, > > > > > > > > > > We could certainly add another API/Lua function but it might be > easier to > > > > > change > > > > > > > > > > luaL_checkinteger(L, 2) in > > > > > > > > > > tmout = MAY_LJMP(luaL_checkinteger(L, 2)) * 1000; > > > > > > > > > > to luaL_checknumber(L, 2), along with appropriate cast to int. > > > > > > > > > > Then we have backwards compatibility, less documentation to write, > and > > > > get > > > > > millisecond timeouts. > > > > > > > > At least it seems important to round up non-null values to the next > > > > millisecond, otherwise we may observe busy loops when users specify > > > > sleep delays smaller than the millisecond, as haproxy's internal > > > > clock is millisecond-based (poll()'s resolution). > > > > > > > > > > > > If people want a separate API, I'm happy to do that too, just more > work. > > > > > > > > I think it should work as you propose it, more or less the round up > of > > > > course. > > > > > > > > > Please advise, and I'll make a patch either way. I'm unfamiliar > with the > > > > > HAProxy development process, so any tips or pointers are welcome, > > > > > > > > It's important to CC the subsystem maintainer when submitting a > change, > > > > since they are supposed to have the last word on submissions in their > > > > area. This is done here since Thierry maintains the Lua area. Please > > > > carefully read CONTRIBUTING in the sources directory, it's not very > > > > long and will help you ensure that all your patches are easily > merged. > > > > And you're welcome to propose changes to this file if something is > > > > unclear :-) > > > > > > > > Thanks, > > > > Willy > > > > > > > > > > > > > ---------- > > > > > > > > > > > > <0001-MINOR-lua-allow-socket-api-settimeout-to-accept-inte.patch> > > > > > > <0001-MINOR-lua-allow-socket-api-settimeout-to-accept-inte.patch> > > >
0001-MINOR-lua-allow-socket-api-settimeout-to-accept-inte.patch
Description: Binary data
