Welcome! Log In Create A New Profile

Advanced

[PATCH] MINOR: Add server name & puid to LUA Server class.

Posted by Patrick Hemmer 
Patrick Hemmer
[PATCH] MINOR: Add server name & puid to LUA Server class.
April 29, 2018 08:30PM
---
doc/lua-api/index.rst | 8 ++++++++
src/hlua_fcn.c | 14 ++++++++++++++
2 files changed, 22 insertions(+)
Hi Thierry,

when you have a moment, could you please give a quick look at these
patches from Patrick so that I know if I can merge them or not ? There
are 2 other ones on the list.

Thanks,
Willy

On Sun, Apr 29, 2018 at 02:23:48PM -0400, Patrick Hemmer wrote:
> ---
> doc/lua-api/index.rst | 8 ++++++++
> src/hlua_fcn.c | 14 ++++++++++++++
> 2 files changed, 22 insertions(+)
>
>

> diff --git a/doc/lua-api/index.rst b/doc/lua-api/index.rst
> index 7a77e46ee..cdb285957 100644
> --- a/doc/lua-api/index.rst
> +++ b/doc/lua-api/index.rst
> @@ -925,6 +925,14 @@ Server class
>
> This class provides a way for manipulating servers and retrieving information.
>
> +.. js:attribute:: Server.name
> +
> + Contain the name of the server.
> +
> +.. js:attribute:: Server.puid
> +
> + Contain the proxy unique identifier of the server.
> +
> .. js:function:: Server.is_draining(sv)
>
> Return true if the server is currently draining sticky connections.
> diff --git a/src/hlua_fcn.c b/src/hlua_fcn.c
> index a8d53d45b..280d8e5af 100644
> --- a/src/hlua_fcn.c
> +++ b/src/hlua_fcn.c
> @@ -490,6 +490,8 @@ int hlua_listener_get_stats(lua_State *L)
>
> int hlua_fcn_new_server(lua_State *L, struct server *srv)
> {
> + char buffer[10];
> +
> lua_newtable(L);
>
> /* Pop a class sesison metatable and affect it to the userdata. */
> @@ -498,6 +500,18 @@ int hlua_fcn_new_server(lua_State *L, struct server *srv)
>
> lua_pushlightuserdata(L, srv);
> lua_rawseti(L, -2, 0);
> +
> + /* Add server name. */
> + lua_pushstring(L, "name");
> + lua_pushstring(L, srv->id);
> + lua_settable(L, -3);
> +
> + /* Add server puid. */
> + lua_pushstring(L, "puid");
> + snprintf(buffer, sizeof(buffer), "%d", srv->puid);
> + lua_pushstring(L, buffer);
> + lua_settable(L, -3);
> +
> return 1;
> }
>
>
> On 2 May 2018, at 16:49, Willy Tarreau <[email protected]> wrote:
>
> Hi Thierry,
>
> when you have a moment, could you please give a quick look at these
> patches from Patrick so that I know if I can merge them or not ? There
> are 2 other ones on the list.


Hi Willy and Patrick,

I check it. I don’t understand why you convert the puid in string.
You could add directly the ouid integer as is in a Lua variable with
the function lua_pushinteger().

Thierry

>
> Thanks,
> Willy
>
> On Sun, Apr 29, 2018 at 02:23:48PM -0400, Patrick Hemmer wrote:
>> ---
>> doc/lua-api/index.rst | 8 ++++++++
>> src/hlua_fcn.c | 14 ++++++++++++++
>> 2 files changed, 22 insertions(+)
>>
>>
>
>> diff --git a/doc/lua-api/index.rst b/doc/lua-api/index.rst
>> index 7a77e46ee..cdb285957 100644
>> --- a/doc/lua-api/index.rst
>> +++ b/doc/lua-api/index.rst
>> @@ -925,6 +925,14 @@ Server class
>>
>> This class provides a way for manipulating servers and retrieving information.
>>
>> +.. js:attribute:: Server.name
>> +
>> + Contain the name of the server.
>> +
>> +.. js:attribute:: Server.puid
>> +
>> + Contain the proxy unique identifier of the server.
>> +
>> .. js:function:: Server.is_draining(sv)
>>
>> Return true if the server is currently draining sticky connections.
>> diff --git a/src/hlua_fcn.c b/src/hlua_fcn.c
>> index a8d53d45b..280d8e5af 100644
>> --- a/src/hlua_fcn.c
>> +++ b/src/hlua_fcn.c
>> @@ -490,6 +490,8 @@ int hlua_listener_get_stats(lua_State *L)
>>
>> int hlua_fcn_new_server(lua_State *L, struct server *srv)
>> {
>> + char buffer[10];
>> +
>> lua_newtable(L);
>>
>> /* Pop a class sesison metatable and affect it to the userdata. */
>> @@ -498,6 +500,18 @@ int hlua_fcn_new_server(lua_State *L, struct server *srv)
>>
>> lua_pushlightuserdata(L, srv);
>> lua_rawseti(L, -2, 0);
>> +
>> + /* Add server name. */
>> + lua_pushstring(L, "name");
>> + lua_pushstring(L, srv->id);
>> + lua_settable(L, -3);
>> +
>> + /* Add server puid. */
>> + lua_pushstring(L, "puid");
>> + snprintf(buffer, sizeof(buffer), "%d", srv->puid);
>> + lua_pushstring(L, buffer);
>> + lua_settable(L, -3);
>> +
>> return 1;
>> }
>>
>>
>
> On 2 May 2018, at 16:49, Willy Tarreau <[email protected]> wrote:
>
> Hi Thierry,
>
> when you have a moment, could you please give a quick look at these
> patches from Patrick so that I know if I can merge them or not ? There
> are 2 other ones on the list.


Hi Willy and Patrick,

I check it. I don’t understand why you convert the puid in string.
You could add directly the ouid integer as is in a Lua variable with
the function lua_pushinteger().

Thierry

>
> Thanks,
> Willy
>
> On Sun, Apr 29, 2018 at 02:23:48PM -0400, Patrick Hemmer wrote:
>> ---
>> doc/lua-api/index.rst | 8 ++++++++
>> src/hlua_fcn.c | 14 ++++++++++++++
>> 2 files changed, 22 insertions(+)
>>
>>
>
>> diff --git a/doc/lua-api/index.rst b/doc/lua-api/index.rst
>> index 7a77e46ee..cdb285957 100644
>> --- a/doc/lua-api/index.rst
>> +++ b/doc/lua-api/index.rst
>> @@ -925,6 +925,14 @@ Server class
>>
>> This class provides a way for manipulating servers and retrieving information.
>>
>> +.. js:attribute:: Server.name
>> +
>> + Contain the name of the server.
>> +
>> +.. js:attribute:: Server.puid
>> +
>> + Contain the proxy unique identifier of the server.
>> +
>> .. js:function:: Server.is_draining(sv)
>>
>> Return true if the server is currently draining sticky connections.
>> diff --git a/src/hlua_fcn.c b/src/hlua_fcn.c
>> index a8d53d45b..280d8e5af 100644
>> --- a/src/hlua_fcn.c
>> +++ b/src/hlua_fcn.c
>> @@ -490,6 +490,8 @@ int hlua_listener_get_stats(lua_State *L)
>>
>> int hlua_fcn_new_server(lua_State *L, struct server *srv)
>> {
>> + char buffer[10];
>> +
>> lua_newtable(L);
>>
>> /* Pop a class sesison metatable and affect it to the userdata. */
>> @@ -498,6 +500,18 @@ int hlua_fcn_new_server(lua_State *L, struct server *srv)
>>
>> lua_pushlightuserdata(L, srv);
>> lua_rawseti(L, -2, 0);
>> +
>> + /* Add server name. */
>> + lua_pushstring(L, "name");
>> + lua_pushstring(L, srv->id);
>> + lua_settable(L, -3);
>> +
>> + /* Add server puid. */
>> + lua_pushstring(L, "puid");
>> + snprintf(buffer, sizeof(buffer), "%d", srv->puid);
>> + lua_pushstring(L, buffer);
>> + lua_settable(L, -3);
>> +
>> return 1;
>> }
>>
>>
>
On 2018/5/3 02:52, Thierry Fournier wrote:
>> On 2 May 2018, at 16:49, Willy Tarreau <[email protected]> wrote:
>>
>> Hi Thierry,
>>
>> when you have a moment, could you please give a quick look at these
>> patches from Patrick so that I know if I can merge them or not ? There
>> are 2 other ones on the list.
>
> Hi Willy and Patrick,
>
> I check it. I don’t understand why you convert the puid in string.
> You could add directly the ouid integer as is in a Lua variable with
> the function lua_pushinteger().
>
> Thierry
I did this for consistency, as this is how Proxy.uuid behaves. Even
though it could return an integer, it converts it to a string and
returns that instead.

>> Thanks,
>> Willy
>>
>> On Sun, Apr 29, 2018 at 02:23:48PM -0400, Patrick Hemmer wrote:
>>> ---
>>> doc/lua-api/index.rst | 8 ++++++++
>>> src/hlua_fcn.c | 14 ++++++++++++++
>>> 2 files changed, 22 insertions(+)
>>>
>>>
>>> diff --git a/doc/lua-api/index.rst b/doc/lua-api/index.rst
>>> index 7a77e46ee..cdb285957 100644
>>> --- a/doc/lua-api/index.rst
>>> +++ b/doc/lua-api/index.rst
>>> @@ -925,6 +925,14 @@ Server class
>>>
>>> This class provides a way for manipulating servers and retrieving information.
>>>
>>> +.. js:attribute:: Server.name
>>> +
>>> + Contain the name of the server.
>>> +
>>> +.. js:attribute:: Server.puid
>>> +
>>> + Contain the proxy unique identifier of the server.
>>> +
>>> .. js:function:: Server.is_draining(sv)
>>>
>>> Return true if the server is currently draining sticky connections.
>>> diff --git a/src/hlua_fcn.c b/src/hlua_fcn.c
>>> index a8d53d45b..280d8e5af 100644
>>> --- a/src/hlua_fcn.c
>>> +++ b/src/hlua_fcn.c
>>> @@ -490,6 +490,8 @@ int hlua_listener_get_stats(lua_State *L)
>>>
>>> int hlua_fcn_new_server(lua_State *L, struct server *srv)
>>> {
>>> + char buffer[10];
>>> +
>>> lua_newtable(L);
>>>
>>> /* Pop a class sesison metatable and affect it to the userdata. */
>>> @@ -498,6 +500,18 @@ int hlua_fcn_new_server(lua_State *L, struct server *srv)
>>>
>>> lua_pushlightuserdata(L, srv);
>>> lua_rawseti(L, -2, 0);
>>> +
>>> + /* Add server name. */
>>> + lua_pushstring(L, "name");
>>> + lua_pushstring(L, srv->id);
>>> + lua_settable(L, -3);
>>> +
>>> + /* Add server puid. */
>>> + lua_pushstring(L, "puid");
>>> + snprintf(buffer, sizeof(buffer), "%d", srv->puid);
>>> + lua_pushstring(L, buffer);
>>> + lua_settable(L, -3);
>>> +
>>> return 1;
>>> }
>>>
>>>
>
> On 3 May 2018, at 14:27, Patrick Hemmer <[email protected]> wrote:
>
>
>
> On 2018/5/3 02:52, Thierry Fournier wrote:
>>> On 2 May 2018, at 16:49, Willy Tarreau <[email protected]>
>>> wrote:
>>>
>>> Hi Thierry,
>>>
>>> when you have a moment, could you please give a quick look at these
>>> patches from Patrick so that I know if I can merge them or not ? There
>>> are 2 other ones on the list.
>>>
>>
>> Hi Willy and Patrick,
>>
>> I check it. I don’t understand why you convert the puid in string.
>> You could add directly the ouid integer as is in a Lua variable with
>> the function lua_pushinteger().
>>
>> Thierry
>>
> I did this for consistency, as this is how Proxy.uuid behaves. Even though it could return an integer, it converts it to a string and returns that instead.


Ok, I understand ... I hesitate. I think that is better to keep
the original type, but the consistency is a good reason to convert
to string.

I doubt that integer operations (add, sub, ...) will be performed on
this id. So maybe, we can keep string for the consistency.

If anyone have an opinion, do not hesitate...

Thierry


>
>>> Thanks,
>>> Willy
>>>
>>> On Sun, Apr 29, 2018 at 02:23:48PM -0400, Patrick Hemmer wrote:
>>>
>>>> ---
>>>> doc/lua-api/index.rst | 8 ++++++++
>>>> src/hlua_fcn.c | 14 ++++++++++++++
>>>> 2 files changed, 22 insertions(+)
>>>>
>>>>
>>>>
>>>> diff --git a/doc/lua-api/index.rst b/doc/lua-api/index.rst
>>>> index 7a77e46ee..cdb285957 100644
>>>> --- a/doc/lua-api/index.rst
>>>> +++ b/doc/lua-api/index.rst
>>>> @@ -925,6 +925,14 @@ Server class
>>>>
>>>> This class provides a way for manipulating servers and retrieving information.
>>>>
>>>> +.. js:attribute:: Server.name
>>>> +
>>>> + Contain the name of the server.
>>>> +
>>>> +.. js:attribute:: Server.puid
>>>> +
>>>> + Contain the proxy unique identifier of the server.
>>>> +
>>>> .. js:function:: Server.is_draining(sv)
>>>>
>>>> Return true if the server is currently draining sticky connections.
>>>> diff --git a/src/hlua_fcn.c b/src/hlua_fcn.c
>>>> index a8d53d45b..280d8e5af 100644
>>>> --- a/src/hlua_fcn.c
>>>> +++ b/src/hlua_fcn.c
>>>> @@ -490,6 +490,8 @@ int hlua_listener_get_stats(lua_State *L)
>>>>
>>>> int hlua_fcn_new_server(lua_State *L, struct server *srv)
>>>> {
>>>> + char buffer[10];
>>>> +
>>>> lua_newtable(L);
>>>>
>>>> /* Pop a class sesison metatable and affect it to the userdata. */
>>>> @@ -498,6 +500,18 @@ int hlua_fcn_new_server(lua_State *L, struct server *srv)
>>>>
>>>> lua_pushlightuserdata(L, srv);
>>>> lua_rawseti(L, -2, 0);
>>>> +
>>>> + /* Add server name. */
>>>> + lua_pushstring(L, "name");
>>>> + lua_pushstring(L, srv->id);
>>>> + lua_settable(L, -3);
>>>> +
>>>> + /* Add server puid. */
>>>> + lua_pushstring(L, "puid");
>>>> + snprintf(buffer, sizeof(buffer), "%d", srv->puid);
>>>> + lua_pushstring(L, buffer);
>>>> + lua_settable(L, -3);
>>>> +
>>>> return 1;
>>>> }
>>>>
>>>>
>>>>
>>
>
Thierry,

Am 03.05.2018 um 15:35 schrieb Thierry Fournier:
> Ok, I understand ... I hesitate. I think that is better to keep
> the original type, but the consistency is a good reason to convert
> to string.
>
> I doubt that integer operations (add, sub, ...) will be performed on
> this id. So maybe, we can keep string for the consistency.
>

I believe that opaque identifiers should be strings always. Not
integers, just because they happen to consist of digits only.

Using strings would also allow you to change the internal data type
later on, without breaking Lua.

So: +1 for string from me.

Best regards
Tim Düsterhus
On Thu, May 03, 2018 at 03:35:41PM +0200, Thierry Fournier wrote:
> Ok, I understand ... I hesitate. I think that is better to keep
> the original type, but the consistency is a good reason to convert
> to string.
>
> I doubt that integer operations (add, sub, ...) will be performed on
> this id. So maybe, we can keep string for the consistency.
>
> If anyone have an opinion, do not hesitate...

OK then I picked it. However I noticed something wrong that I fixed
and which is wrong in other parts of the code :

char buffer[10];
(...)
snprintf(buffer, sizeof(buffer), "%d", srv->puid);
lua_pushstring(L, buffer);

With UIDs larger than 999999999 snprintf() will fail and we'll push a
random string in the buffer. So I fixed it to set the buffer size to
12 ("-2147483648"). I'll have to fix the other places in the same code
part for this as well.

It's extremely unlikely anyone will notice it since few people use manual
IDs, but if some automated tool builds these IDs using ranges, this will
definitely fail.

thanks!
Willy
> On 3 May 2018, at 18:49, Willy Tarreau <[email protected]> wrote:
>
> On Thu, May 03, 2018 at 03:35:41PM +0200, Thierry Fournier wrote:
>> Ok, I understand ... I hesitate. I think that is better to keep
>> the original type, but the consistency is a good reason to convert
>> to string.
>>
>> I doubt that integer operations (add, sub, ...) will be performed on
>> this id. So maybe, we can keep string for the consistency.
>>
>> If anyone have an opinion, do not hesitate...
>
> OK then I picked it. However I noticed something wrong that I fixed
> and which is wrong in other parts of the code :
>
> char buffer[10];
> (...)
> snprintf(buffer, sizeof(buffer), "%d", srv->puid);
> lua_pushstring(L, buffer);
>
> With UIDs larger than 999999999 snprintf() will fail and we'll push a
> random string in the buffer. So I fixed it to set the buffer size to
> 12 ("-2147483648"). I'll have to fix the other places in the same code
> part for this as well.
>
> It's extremely unlikely anyone will notice it since few people use manual
> IDs, but if some automated tool builds these IDs using ranges, this will
> definitely fail.


Thanks Willy.
Thierry
Sorry, only registered users may post in this forum.

Click here to login