Обсуждение: wrong comment in libpq.h

Hi Hackers,

There is a comment like below in src/include/libpq/libpq.h,

  /*
   * prototypes for functions in be-secure.c
   */
extern PGDLLIMPORT char *ssl_library;
extern PGDLLIMPORT char *ssl_cert_file;

...

However, 'ssl_library', 'ssl_cert_file' and the rest are global 
parameter settings, not functions. To address this confusion, it would 
be better to move all global configuration settings to the proper 
section, such as /* GUCs */, to maintain consistency.

I have attached an attempt to help address this issue.


Thank you,

David

On 03.05.24 00:37, David Zhang wrote:
> Hi Hackers,
> 
> There is a comment like below in src/include/libpq/libpq.h,
> 
>   /*
>    * prototypes for functions in be-secure.c
>    */
> extern PGDLLIMPORT char *ssl_library;
> extern PGDLLIMPORT char *ssl_cert_file;
> 
> ...
> 
> However, 'ssl_library', 'ssl_cert_file' and the rest are global 
> parameter settings, not functions. To address this confusion, it would 
> be better to move all global configuration settings to the proper 
> section, such as /* GUCs */, to maintain consistency.

Maybe it's easier if we just replaced

     prototypes for functions in xxx.c

with

     declarations for xxx.c

throughout src/include/libpq/libpq.h.

> On 3 May 2024, at 13:48, Peter Eisentraut <peter@eisentraut.org> wrote:

> Maybe it's easier if we just replaced
> 
>    prototypes for functions in xxx.c
> 
> with
> 
>    declarations for xxx.c
> 
> throughout src/include/libpq/libpq.h.

+1

--
Daniel Gustafsson

On 2024-05-03 4:53 a.m., Daniel Gustafsson wrote:
>> On 3 May 2024, at 13:48, Peter Eisentraut <peter@eisentraut.org> wrote:
>> Maybe it's easier if we just replaced
>>
>>     prototypes for functions in xxx.c
>>
>> with
>>
>>     declarations for xxx.c
>>
>> throughout src/include/libpq/libpq.h.
> +1
+1
>
> --
> Daniel Gustafsson
>
David

On 04.05.24 00:29, David Zhang wrote:
> On 2024-05-03 4:53 a.m., Daniel Gustafsson wrote:
>>> On 3 May 2024, at 13:48, Peter Eisentraut <peter@eisentraut.org> wrote:
>>> Maybe it's easier if we just replaced
>>>
>>>     prototypes for functions in xxx.c
>>>
>>> with
>>>
>>>     declarations for xxx.c
>>>
>>> throughout src/include/libpq/libpq.h.
>> +1
> +1

It looks like this wording "prototypes for functions in" is used many 
times in src/include/, in many cases equally inaccurately, so I would 
suggest creating a more comprehensive patch for this.

> It looks like this wording "prototypes for functions in" is used many 
> times in src/include/, in many cases equally inaccurately, so I would 
> suggest creating a more comprehensive patch for this.

I noticed this "prototypes for functions in" in many header files and 
briefly checked them. It kind of all make sense except the bufmgr.h has 
something like below.

/* in buf_init.c */
extern void InitBufferPool(void);
extern Size BufferShmemSize(void);

/* in localbuf.c */
extern void AtProcExit_LocalBuffers(void);

/* in freelist.c */

which doesn't say "prototypes for functions xxx", but it still make 
sense for me.


The main confusion part is in libpq.h.

/*
  * prototypes for functions in be-secure.c
  */
extern PGDLLIMPORT char *ssl_library;
extern PGDLLIMPORT char *ssl_cert_file;
extern PGDLLIMPORT char *ssl_key_file;
extern PGDLLIMPORT char *ssl_ca_file;
extern PGDLLIMPORT char *ssl_crl_file;
extern PGDLLIMPORT char *ssl_crl_dir;
extern PGDLLIMPORT char *ssl_dh_params_file;
extern PGDLLIMPORT char *ssl_passphrase_command;
extern PGDLLIMPORT bool ssl_passphrase_command_supports_reload;
#ifdef USE_SSL
extern PGDLLIMPORT bool ssl_loaded_verify_locations;
#endif

If we can delete the comment and move the variables declarations to /* 
GUCs */ section, then it should be more consistent.

/* GUCs */
extern PGDLLIMPORT char *SSLCipherSuites;
extern PGDLLIMPORT char *SSLECDHCurve;
extern PGDLLIMPORT bool SSLPreferServerCiphers;
extern PGDLLIMPORT int ssl_min_protocol_version;
extern PGDLLIMPORT int ssl_max_protocol_version;

One more argument for my previous patch is that with this minor change 
it can better align with the parameters in postgresql.conf.

# - SSL -

#ssl = off
#ssl_ca_file = ''
#ssl_cert_file = 'server.crt'
#ssl_crl_file = ''
#ssl_crl_dir = ''
#ssl_key_file = 'server.key'
#ssl_ciphers = 'HIGH:MEDIUM:+3DES:!aNULL'    # allowed SSL ciphers
#ssl_prefer_server_ciphers = on
#ssl_ecdh_curve = 'prime256v1'
#ssl_min_protocol_version = 'TLSv1.2'
#ssl_max_protocol_version = ''
#ssl_dh_params_file = ''
#ssl_passphrase_command = ''
#ssl_passphrase_command_supports_reload = off


best regards,

David