[#392] Add application name and version in management protocol

[EuGig]

In order to improve debugging, [#392] suggested to write the sender application name and its version into the header over which pgagroal-cli and pgagroal communicate.

Introduces the write_header_info function in management.c

Every time a pgagroal-cli is used, cli.c writes into the communication socket through the write_header function. Then pgagroal replies with one of the pgagroal_write_ functions, depending on the option specified. Therefore, the strategy is to call write_header_info into both.

Arguments of write_header_info:

• char* header: the header to write sender application name and its version into
• bool start: In case the function is called in write_header, the block is written after type and slot.
• char* command: server application name.

As for now, this commit adds write_header_info only in the pgagroal_write_isalive function called if the user specified option ping while calling the pgagroal-cli command.

The plan is to insert write_header_info into every pgagroal_write_ function called by main.c, if the strategy is successful and after code reviews.

[EuGig]

1637 pgagroal_log_debug("Header: %s\n", header); could be the cause of the segmentation fault, since header has also type and slot which are not strings.

Oh yeah, calling write_header_info inside read-header can save us some coding. Though, can calling a "writing" function inside another function which is supposed to only read something maybe be a bit confusing?

[fluca1978]

1637 pgagroal_log_debug("Header: %s\n", header); could be the cause of the segmentation fault, since header has also type and slot which are not strings.

No it's not, even if that line could lead to other problems. The line:

   else {
      pgagroal_write_string(header + 4, header_info);
   }

is the cause, and it's probably because hedaer + 4 is wrong (not allocated when you expect it). I've digged anymore to understand why, but it's not clear to me why you are writing the header in different positions.

Oh yeah, calling write_header_info inside read-header can save us some coding. Though, can calling a "writing" function inside another function which is supposed to only read something maybe be a bit confusing?

No, there is some confusion originating from me: write_header already calls write_header_info, which is what I would expect. I don't get why you should call it also from read_header.

[EuGig]

I've digged anymore to understand why, but it's not clear to me why you are writing the header in different positions.

The problem is that write_header writes at the beginning of the header the type and the slot, i.e., 5 bytes. So to not loose this information I thought to write starting from header + 4. However, when pgagroal writes in the socket, it seems it does not include this information anymore.

[EuGig]

No, there is some confusion originating from me: write_header already calls write_header_info, which is what I would expect. I don't get why you should call it also from read_header.

I also got confused. After reflecting a bit, this is what I think can work:

1. cli.c calls a function based on a specified option, say isalive, which calls write_header (info about pgagroal-cli are written in the header)
2. main.c reads the header through pgagroal_management_read_header and prints for debugging purposes info about pgagraol-cli.
3. main.c writes the respond through pgagroal_write_isalive (info about pgagroal are written in the header)
4. cli.c reads the header through pgagroal_read_isalive and prints for debugging purposes info about pgagroal.

So, maybe the plan can be summarised as follows:

• write_header_info must be called inside write_header and pgagroal_write_isalive.
• pgagroal_log_debug must be called inside pgagroal_management_read_header and pgagroal_management_read_isalive.

Maybe I'm missing something and maybe we don't need to insert write_header_info inside every pgagroal_write_* function. Does that make sense to you?

[fluca1978]

No, there is some confusion originating from me: write_header already calls write_header_info, which is what I would expect. I don't get why you should call it also from read_header.

I also got confused. After reflecting a bit, this is what I think can work:

1. cli.c calls a function based on a specified option, say `isalive`, which calls `write_header` (info about pgagroal-cli are written in the header)

2. main.c reads the header through `pgagroal_management_read_header` and prints for debugging purposes info about pgagraol-cli.

3. main.c writes the respond through `pgagroal_write_isalive` (info about pgagroal are written in the header)

4. cli.c reads the header through `pgagroal_read_isalive` and prints for debugging purposes info about pgagroal.

The above is correct. The fact that the header is written only from one side is why, initially, I thought that placing the information into the payload was simpler than placing into an header. In other words, the write_info_header function is inserting something in the header when the pgagroal-cli requests a command, but not when pgagroal is answering.

So, maybe the plan can be summarised as follows:

* `write_header_info` must be called inside `write_header` and `pgagroal_write_isalive`.

Correct.

* `pgagroal_log_debug` must be called inside `pgagroal_management_read_header` and `pgagroal_management_read_isalive`.

No. The purpose of this work is not to debug who is sending a message, but giving the counterpart to be able to understand who sent the message. While you can add as much logging as you want, the final aim is to make pgagroal-cli and pgagroal able to understand who originated the message they are handling.

Maybe I'm missing something and maybe we don't need to insert write_header_info inside every pgagroal_write_* function. Does that make sense to you?

I think we need to add into every write function, because we need to provide such information in both directions.

[EuGig]

In other words, the write_info_header function is inserting something in the header when the pgagroal-cli requests a command, but not when pgagroal is answering.

As a matter of fact, I'm finding problems in adding write_header_info inside pgagroal_management_write_status and pgagroal_management_write_details. Actually, this two functions write something in the header, so one has to play with offsets a little bit too much. Accordingly, one has to change offsets in pgagroal_management_read_status and pgagroal_management_read_status_details too.

[jesperpedersen]

You could look into expanding the pgagroal_append() infrastructure and use that as the base

[fluca1978]

In other words, the write_info_header function is inserting something in the header when the pgagroal-cli requests a command, but not when pgagroal is answering.

As a matter of fact, I'm finding problems in adding write_header_info inside pgagroal_management_write_status and pgagroal_management_write_details. Actually, this two functions write something in the header, so one has to play with offsets a little bit too much. Accordingly, one has to change offsets in pgagroal_management_read_status and pgagroal_management_read_status_details too.

@EuGig did you manage to add the information header in all the other cases? We can study better the status and details commands.

[EuGig]

You could look into expanding the pgagroal_append() infrastructure and use that as the base

Thank you for the suggestion @jesperpedersen! I actually used offsets instead.

@EuGig did you manage to add the information header in all the other cases? We can study better the status and details commands.

Yes! In the last commit I did the following:

• MANAGEMENT_HEADER_SIZE is 23. (length of "S:pgagroal-cli;V:M.m.p")
• Instead of a boolean parameter I used offset to decide where to write it.

In general, if a header was defined inside the function, I augmented the header size and wrote info at the end of it. Otherwise, like in pgagroal_management_write_conf_ls and pgagroal_management_write_config_get I wrote it at the end of the payload since a header was not defined. I know this can cause some confusion, since application name and version are sometimes written in the header and sometimes in the payload. But this seemed to me the less painful change, because there is no need to change also the corresponding pgagroal_management_read_* functions. Maybe we can think of changing the name of write_header_info to write_info.

I cannot get this working, even if it compiles

I hope you will be able run it this time.

Changes in pgagroal.h are due to uncrustify.

[jesperpedersen]

Header should be in the start of the payload for sure

[EuGig]

Just rebased after 5dcfb897d90b78c622a38855ae6ffd5cb2630d9f fixing [#407].

[jesperpedersen]

You need to rebase

https://github.com/progit/progit2/releases

[jesperpedersen]

You need to add your name to AUTHORS too

[EuGig]

Thanks, will do!

[jesperpedersen]

See build failure...

[jesperpedersen]

https://github.com/agroal/pgagroal/blob/master/contrib/valgrind/README.md

[fluca1978]

I see you added a few piece that looks like:

 if (version_buf != NULL)
   {
      pgagroal_executable_version_name(version_buf, version);

      cJSON* app_json = cJSON_CreateObject();
      cJSON_AddItemToObject(output, "application", app_json);
      cJSON_AddStringToObject(app_json, "name", pgagroal_executable_name(application));
      cJSON_AddStringToObject(app_json, "version", version_buf);
      free(version_buf);
   }

but we already have pgagroal_json_create_new_command_object that accepts the executable name, so I think it would be better to extend such prototype to include also the application version, so you need to extract such information first from the buffer and then create the JSON object with all the stuff altready packed. Note that pgagroal_json_create_new_command_object already has the "application" and "version" entries, so you need only to customize their values.

Moreover, I'm unable to compile:

% make clean && cmake .. && make && sudo make install
-- pgagroal 1.7.0
-- Build type is Release
-- System is Linux
-- libev found
-- OpenSSL found
-- rst2man found
-- cJSON found version 1.7.17
-- libatomic found
-- systemd found
-- Configuring done
-- Generating done
-- Build files have been written to: /home/luca/pgagroal/build
[  0%] Built target man
[  3%] Building C object src/CMakeFiles/pgagroal.dir/libpgagroal/configuration.c.o
[  7%] Building C object src/CMakeFiles/pgagroal.dir/libpgagroal/json.c.o
[ 10%] Building C object src/CMakeFiles/pgagroal.dir/libpgagroal/logging.c.o
[ 14%] Building C object src/CMakeFiles/pgagroal.dir/libpgagroal/management.c.o
/home/luca/pgagroal/src/libpgagroal/management.c: In function ‘pgagroal_management_json_read_status_details’:
/home/luca/pgagroal/src/libpgagroal/management.c:1919:20: error: array subscript 5 is outside array bounds of ‘char[5]’ [-Werror=array-bounds]
 1919 |    version_name[5] = '\0';
      |    ~~~~~~~~~~~~~~~~^~~~~~
/home/luca/pgagroal/src/libpgagroal/management.c:739:31: note: referencing an object of size 5 allocated by ‘malloc’
  739 |    char* version_buf = (char*)malloc(5 * sizeof(char));
      |                               ^~~~~~~~~~~~~~~~~~~~~~~~
/home/luca/pgagroal/src/libpgagroal/management.c: In function ‘pgagroal_management_read_isalive’:
/home/luca/pgagroal/src/libpgagroal/management.c:1919:20: error: array subscript 5 is outside array bounds of ‘char[5]’ [-Werror=array-bounds]
 1919 |    version_name[5] = '\0';
      |    ~~~~~~~~~~~~~~~~^~~~~~
/home/luca/pgagroal/src/libpgagroal/management.c:1220:38: note: referencing an object of size 5 allocated by ‘malloc’
 1220 |           char* version_buf = (char*)malloc(5 * sizeof(char));
      |                                      ^~~~~~~~~~~~~~~~~~~~~~~~
/home/luca/pgagroal/src/libpgagroal/management.c: In function ‘pgagroal_management_read_config_get’:
/home/luca/pgagroal/src/libpgagroal/management.c:1919:20: error: array subscript 5 is outside array bounds of ‘char[5]’ [-Werror=array-bounds]
 1919 |    version_name[5] = '\0';
      |    ~~~~~~~~~~~~~~~~^~~~~~
/home/luca/pgagroal/src/libpgagroal/management.c:2134:31: note: referencing an object of size 5 allocated by ‘malloc’
 2134 |    char* version_buf = (char*)malloc(5 * sizeof(char));
      |                               ^~~~~~~~~~~~~~~~~~~~~~~~
/home/luca/pgagroal/src/libpgagroal/management.c: In function ‘pgagroal_management_read_conf_ls’:
/home/luca/pgagroal/src/libpgagroal/management.c:1919:20: error: array subscript 5 is outside array bounds of ‘char[5]’ [-Werror=array-bounds]
 1919 |    version_name[5] = '\0';
      |    ~~~~~~~~~~~~~~~~^~~~~~
/home/luca/pgagroal/src/libpgagroal/management.c:2751:31: note: referencing an object of size 5 allocated by ‘malloc’
 2751 |    char* version_buf = (char*)malloc(5 * sizeof(char));
      |                               ^~~~~~~~~~~~~~~~~~~~~~~~
/home/luca/pgagroal/src/libpgagroal/management.c:2857:4: error: ‘buffer’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
 2857 |    free(buffer);
      |    ^~~~~~~~~~~~
/home/luca/pgagroal/src/libpgagroal/management.c:2735:10: note: ‘buffer’ was declared here
 2735 |    char* buffer;
      |          ^~~~~~
cc1: all warnings being treated as errors
make[2]: *** [src/CMakeFiles/pgagroal.dir/build.make:118: src/CMakeFiles/pgagroal.dir/libpgagroal/management.c.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:254: src/CMakeFiles/pgagroal.dir/all] Error 2
make: *** [Makefile:156: all] Error 2

Last, there is the need to uncrustify the code: there are places where the code is not aligned and other lines that present only a tab character.

Can you please have a look at all the above?

Thanls

[EuGig]

Hi @fluca1978!

but we already have pgagroal_json_create_new_command_object that accepts the executable name, so I think it would be better to extend such prototype to include also the application version, so you need to extract such information first from the buffer and then create the JSON object with all the stuff altready packed. Note that pgagroal_json_create_new_command_object already has the "application" and "version" entries, so you need only to customize their values.

The problem with pgagroal_json_create_new_command_object is that it comes not only with application and version objects, but also with the command and the output object. So, if I add another json object through pgagroal_json_create_new_command_object I got something like this:

{
    "command":  {
        "name": "conf ls",
        "status":   "OK",
        "error":    0,
        "exit-status":  0,
        "output":   {
            "application":  {
                "command":  {
                    "name": "conf ls",
                    "status":   "OK",
                    "error":    0,
                    "exit-status":  0,
                    "output":   {
                    }
                },
                "application":  {
                    "name": "pgagroal",
                    "major":    1,
                    "minor":    7,
                    "patch":    0,
                    "version":  "1.7.0"
                }
            },
            "files":    {
                "list": [{
                        "description":  "Main Configuration file",
                        "path": "pgagroal.conf"
                    }, {
                        "description":  "HBA File",
                        "path": "pgagroal_hba.conf"
                    }, {
                        "description":  "Limit file",
                        "path": ""
                    }, {
                        "description":  "Frontend users file",
                        "path": ""
                    }, {
                        "description":  "Admins file",
                        "path": ""
                    }, {
                        "description":  "Superuser file",
                        "path": ""
                    }, {
                        "description":  "Users file",
                        "path": ""
                    }]
            }
        }
    },
    "application":  {
        "name": "pgagroal-cli",
        "major":    1,
        "minor":    7,
        "patch":    0,
        "version":  "1.7.0"
    }
}

(For simplicity I put it into the output entry, since it is exposed with the pgagroal_json_extract_command_output_object function). So, we should expand this prototype to include the application-version, and also teach it to not print the command entry in case we don't want to repeat it.

[fluca1978]

I meant to use pgagroal_json_create_new_command_object after you dereference the application and version. For example, in pgagroal_management_json_read_conf_ls you have:

   cJSON* json = pgagroal_json_create_new_command_object("conf ls", true, "pgagroal-cli");
   cJSON* output = pgagroal_json_extract_command_output_object(json);

   if (read_complete(ssl, socket, &buf_info[0], sizeof(buf_info)))
   {
      goto error;
   }

   application = pgagroal_read_int32(&(buf_info[2]));
   version = pgagroal_read_int32(&(buf_info[9]));

   char* version_buf = (char*)malloc(5 * sizeof(char));
   if (version_buf != NULL)
   {  ... }

that should be like:

   if (read_complete(ssl, socket, &buf_info[0], sizeof(buf_info)))
   {
      goto error;
   }

   application = pgagroal_read_int32(&(buf_info[2]));
   version = pgagroal_read_int32(&(buf_info[9]));

   cJSON* json = pgagroal_json_create_new_command_object("conf ls", true, application, version);  // <- HERE !
   cJSON* output = pgagroal_json_extract_command_output_object(json);

so you need to add the version parameter to the pgagroal_json_create_command_object and place the appropriate arguments. What do you think?

[EuGig]

Oh I see! Sounds good.

[fluca1978]

I still cannot compile:

% make clean && cmake .. && make && sudo make install
-- pgagroal 1.7.0
-- Build type is Release
-- System is Linux
-- libev found
-- OpenSSL found
-- rst2man found
-- cJSON found version 1.7.17
-- libatomic found
-- systemd found
-- Configuring done
-- Generating done
-- Build files have been written to: /home/luca/pgagroal/build
[  0%] Built target man
[  3%] Building C object src/CMakeFiles/pgagroal.dir/libpgagroal/configuration.c.o
[  7%] Building C object src/CMakeFiles/pgagroal.dir/libpgagroal/json.c.o
[ 10%] Building C object src/CMakeFiles/pgagroal.dir/libpgagroal/logging.c.o
[ 14%] Building C object src/CMakeFiles/pgagroal.dir/libpgagroal/management.c.o
/home/luca/pgagroal/src/libpgagroal/management.c: In function ‘pgagroal_management_json_read_status_details’:
/home/luca/pgagroal/src/libpgagroal/management.c:935:4: error: ‘json’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  935 |    pgagroal_json_set_command_object_faulty(json, strerror(errno), errno);
      |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/luca/pgagroal/src/libpgagroal/management.c: In function ‘pgagroal_management_read_conf_ls’:
/home/luca/pgagroal/src/libpgagroal/management.c:2825:4: error: ‘json’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
 2825 |    pgagroal_json_set_command_object_faulty(json, strerror(errno), errno);
      |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/luca/pgagroal/src/libpgagroal/management.c:2730:11: note: ‘json’ was declared here
 2730 |    cJSON* json = pgagroal_json_create_new_command_object("conf ls", true, pgagroal_executable_name(application), version_buf);
      |           ^~~~
/home/luca/pgagroal/src/libpgagroal/management.c:2828:4: error: ‘version_buf’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
 2828 |    free(version_buf);
      |    ^~~~~~~~~~~~~~~~~
/home/luca/pgagroal/src/libpgagroal/management.c:2721:10: note: ‘version_buf’ was declared here
 2721 |    char* version_buf = (char*)malloc(6 * sizeof(char));
      |          ^~~~~~~~~~~
/home/luca/pgagroal/src/libpgagroal/management.c:2822:4: error: ‘buffer’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
 2822 |    free(buffer);
      |    ^~~~~~~~~~~~
/home/luca/pgagroal/src/libpgagroal/management.c:2708:10: note: ‘buffer’ was declared here
 2708 |    char* buffer;
      |          ^~~~~~
cc1: all warnings being treated as errors
make[2]: *** [src/CMakeFiles/pgagroal.dir/build.make:118: src/CMakeFiles/pgagroal.dir/libpgagroal/management.c.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:254: src/CMakeFiles/pgagroal.dir/all] Error 2
make: *** [Makefile:156: all] Error 2

[EuGig]

Oh sorry, I fixed this error. Though, I wonder why my compiler didn't see this.

[fluca1978]

Looks fine to me. @jesperpedersen PTAL commit d7c6409

[jesperpedersen]

@EuGig Look at the CI failures

[fluca1978]

Does compile on gcc 11.4, interesting there are failures. @EuGig it should suffice to keep the values as long when calling strtol and cast them as integers when needed at the last time.

[jesperpedersen]

Merged.

Thanks for your contribution !