Re: [HACKERS] [PATCH 1/2 v3] [libpq] rework sigpipe-handling macros
On Mon, Jul 20, 2009 at 3:14 AM, Jeremy Kerrj...@ozlabs.org wrote:
That code is not broken as it stands, and doesn't appear to really
gain anything from the proposed change. Why should we risk any
portability questions when the code isn't going to get either simpler
or shorter?
OK, after attempting a macro version of this, we end up with:
#define DECLARE_SIGPIPE_INFO(var) struct sigpipe_info var;
#define DISABLE_SIGPIPE(info, conn) \
((SIGPIPE_MASKED(conn)) ? 0 : \
((info)-got_epipe = false, \
pg_block_sigpipe((info)-oldsigmask,
(info)-sigpipe_pending)) 0)
- kinda ugly, uses the ?: and , operators to return the result of
pg_block_sigpipe. We could avoid the comma with a block though.
If we want to keep the current 'failaction' style of macro:
#define DISABLE_SIGPIPE(info, conn, failaction) \
do { \
if (!SIGPIPE_MASKED(conn)) { \
(info)-got_epipe = false; \
if (pg_block_sigpipe((info)-oldsigmask, \
(info)-sigpipe_pending)) 0) { \
failaction; \
} \
} \
} while (0)
We could ditch struct sigpipe info, but that means we need to declare
three separate vars in DECLARE_SIGPIPE_INFO() instead of the one, and it
doesn't clean up the macro code much. I'd rather reduce the amount of
stuff that we declare behind the caller's back.
Compared to the static-function version:
static inline int disable_sigpipe(PGconn *conn, struct sigpipe_info
*info)
{
if (sigpipe_masked(conn))
return 0;
info-got_epipe = false;
return pq_block_sigpipe(info-oldsigmask, info-sigpipe_pending) 0;
}
Personally, I think the static functions are a lot cleaner, and don't
think we lose any portability from using these (since inline is #define-
ed out on compilers that don't support it). On non-inlining compilers,
we do gain an extra function call, but we're about to enter the kernel
anyway, so this will probably be lost in the noise (especially if we
save the sigpipe-masking syscalls).
But in the end, it's not up to me - do you still prefer the macro
approach?
Since Tom seems to prefer the macro approach, and since the current
code uses a macro, I think we should stick with doing it that way.
Also, as some of Tom's comments above indicate, I don't think it's
making anything any easier for anyone that you keep submitting this as
two separate patches. It's one thing to submit a patch in pieces of
it is very large or complex and especially if the pieces are
independent, but that's not really the case here.
Because we are now over a week into this CommitFest, we need to get a
final, reviewable version of this patch as quickly as possible. So
please make the requested changes and resubmit as soon as you can.
Thanks,
...Robert
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH 1/2 v3] [libpq] rework sigpipe-handling macros
Hi Tom,
That code is not broken as it stands, and doesn't appear to really
gain anything from the proposed change. Why should we risk any
portability questions when the code isn't going to get either simpler
or shorter?
OK, after attempting a macro version of this, we end up with:
#define DECLARE_SIGPIPE_INFO(var) struct sigpipe_info var;
#define DISABLE_SIGPIPE(info, conn) \
((SIGPIPE_MASKED(conn)) ? 0 : \
((info)-got_epipe = false, \
pg_block_sigpipe((info)-oldsigmask,
(info)-sigpipe_pending)) 0)
- kinda ugly, uses the ?: and , operators to return the result of
pg_block_sigpipe. We could avoid the comma with a block though.
If we want to keep the current 'failaction' style of macro:
#define DISABLE_SIGPIPE(info, conn, failaction) \
do { \
if (!SIGPIPE_MASKED(conn)) { \
(info)-got_epipe = false; \
if (pg_block_sigpipe((info)-oldsigmask, \
(info)-sigpipe_pending)) 0) { \
failaction; \
} \
} \
} while (0)
We could ditch struct sigpipe info, but that means we need to declare
three separate vars in DECLARE_SIGPIPE_INFO() instead of the one, and it
doesn't clean up the macro code much. I'd rather reduce the amount of
stuff that we declare behind the caller's back.
Compared to the static-function version:
static inline int disable_sigpipe(PGconn *conn, struct sigpipe_info
*info)
{
if (sigpipe_masked(conn))
return 0;
info-got_epipe = false;
return pq_block_sigpipe(info-oldsigmask, info-sigpipe_pending) 0;
}
Personally, I think the static functions are a lot cleaner, and don't
think we lose any portability from using these (since inline is #define-
ed out on compilers that don't support it). On non-inlining compilers,
we do gain an extra function call, but we're about to enter the kernel
anyway, so this will probably be lost in the noise (especially if we
save the sigpipe-masking syscalls).
But in the end, it's not up to me - do you still prefer the macro
approach?
Cheers,
Jeremy
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH 1/2 v3] [libpq] rework sigpipe-handling macros
Hi Robert, Perhaps we should use macros. I was trying to avoid macros, as this means we lose type- and syntax- checking at the call-site, and end up with slightly messier code. However, I understand that this is probably personal preference for me :) How about just 'static' functions? (ie, drop the 'inline'). This way, the compiler is free to inline where suitable, and non-inlining compilers will do the right thing too. However, I'd rather make decisions on data, rather than guessing. Is the actual problem here that some compilers just don't support the 'inline' keyword? Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH 1/2 v3] [libpq] rework sigpipe-handling macros
Jeremy Kerr j...@ozlabs.org writes: However, I'd rather make decisions on data, rather than guessing. Is the actual problem here that some compilers just don't support the 'inline' keyword? I think Alvaro's complaint is unfounded --- we already have logic to #define inline as empty if the compiler doesn't support it. The issue he's thinking of is that non-gcc compilers typically don't react very well to static function definitions in .h files. However that doesn't apply to the proposed usage, since they're not going to be in a .h file. However, I think the whole patch is pretty useless. That code is not broken as it stands, and doesn't appear to really gain anything from the proposed change. Why should we risk any portability questions when the code isn't going to get either simpler or shorter? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH 1/2 v3] [libpq] rework sigpipe-handling macros
Tom, However, I think the whole patch is pretty useless. That code is not broken as it stands, and doesn't appear to really gain anything from the proposed change. Why should we risk any portability questions when the code isn't going to get either simpler or shorter? This patch clears the way for the proceeding change (2/2). We use the new inline functions to implement the proper checks to see if the sigpipe-masking syscalls are needed. We also need disable_sigpipe to be called when it's not the start of a block, hence the separate type definition. Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH 1/2 v3] [libpq] rework sigpipe-handling macros
Jeremy Kerr j...@ozlabs.org writes: We also need disable_sigpipe to be called when it's not the start of a block, hence the separate type definition. So break that macro into two (variable definition and code). regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH 1/2 v3] [libpq] rework sigpipe-handling macros
Jeremy Kerr wrote: Also, since we don't need to declare variables in the macros, we can change the code to be implemented as static inlines. Does this work in compilers other than GCC? I think we use some kludges to protect against them ... see pg_list.h for the canonical example. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH 1/2 v3] [libpq] rework sigpipe-handling macros
Hi, Alvaro, Does this work in compilers other than GCC? I think we use some kludges to protect against them ... see pg_list.h for the canonical example. As I understand it, we're not using static inlines in pg_list.h to prevent multiple objects from exporting the same symbols if the functions don't end up as 'static inline' (ie, because the compiler doesn't support that). In this case, we're only compiling the inlines into a single object, so even if the compiler doesn't support inlines, we'll just end up with out-of-line function calls, which should work too. However, this is only my assumption about those compilers (I don't have access to other compilers to test); happy to fix these up if the inlines won't work. Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH 1/2 v3] [libpq] rework sigpipe-handling macros
On Tue, Jul 14, 2009 at 8:42 PM, Jeremy Kerrj...@ozlabs.org wrote: Hi, Alvaro, Does this work in compilers other than GCC? I think we use some kludges to protect against them ... see pg_list.h for the canonical example. As I understand it, we're not using static inlines in pg_list.h to prevent multiple objects from exporting the same symbols if the functions don't end up as 'static inline' (ie, because the compiler doesn't support that). In this case, we're only compiling the inlines into a single object, so even if the compiler doesn't support inlines, we'll just end up with out-of-line function calls, which should work too. Perhaps we should use macros. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH 1/2 v3] [libpq] rework sigpipe-handling macros
Currently, the sigpipe-masking code in libpq is implemented as
a set of macros, which depend on declaring local variables.
This change adds a (private) struct sigpipe_info to contain the
compile-dependent data required for sigpipe masking and restoring.
The caller can then declare a struct sigpipe info explicitly, and
pass this to the subsequent sigpipe-masking code.
This allows us to separate the variable declarations from the code,
and gives the caller more flexibility for controlling the scope of
these variables.
Also, since we don't need to declare variables in the macros, we
can change the code to be implemented as static inlines.
Signed-off-by: Jeremy Kerr j...@ozlabs.org
---
src/interfaces/libpq/fe-secure.c | 88 ---
1 file changed, 55 insertions(+), 33 deletions(-)
*** a/src/interfaces/libpq/fe-secure.c
--- b/src/interfaces/libpq/fe-secure.c
***
*** 119,163 static long win32_ssl_create_mutex = 0;
/*
* Macros to handle disabling and then restoring the state of SIGPIPE
handling.
- * Note that DISABLE_SIGPIPE() must appear at the start of a block.
*/
#ifndef WIN32
#ifdef ENABLE_THREAD_SAFETY
! #define DISABLE_SIGPIPE(failaction) \
! sigset_tosigmask; \
! boolsigpipe_pending; \
! boolgot_epipe = false; \
! \
! if (pq_block_sigpipe(osigmask, sigpipe_pending) 0) \
! failaction
! #define REMEMBER_EPIPE(cond) \
! do { \
! if (cond) \
! got_epipe = true; \
! } while (0)
! #define RESTORE_SIGPIPE() \
! pq_reset_sigpipe(osigmask, sigpipe_pending, got_epipe)
#else /* !ENABLE_THREAD_SAFETY */
! #define DISABLE_SIGPIPE(failaction) \
! pqsigfunc oldsighandler = pqsignal(SIGPIPE, SIG_IGN)
! #define REMEMBER_EPIPE(cond)
! #define RESTORE_SIGPIPE() \
! pqsignal(SIGPIPE, oldsighandler)
#endif/* ENABLE_THREAD_SAFETY */
#else /* WIN32 */
! #define DISABLE_SIGPIPE(failaction)
! #define REMEMBER_EPIPE(cond)
! #define RESTORE_SIGPIPE()
#endif/* WIN32 */
--- 119,180
/*
* Macros to handle disabling and then restoring the state of SIGPIPE
handling.
*/
#ifndef WIN32
#ifdef ENABLE_THREAD_SAFETY
! struct sigpipe_info {
! sigset_toldsigmask;
! boolsigpipe_pending;
! boolgot_epipe;
! };
! static inline int disable_sigpipe(struct sigpipe_info *info)
! {
! info-got_epipe = false;
! return pq_block_sigpipe(info-oldsigmask, info-sigpipe_pending) 0;
! }
! static inline void remember_epipe(struct sigpipe_info *info, bool cond)
! {
! if (cond)
! info-got_epipe = true;
! }
!
! static inline void restore_sigpipe(struct sigpipe_info *info)
! {
! pq_reset_sigpipe(info-oldsigmask, info-sigpipe_pending,
info-got_epipe);
! }
#else /* !ENABLE_THREAD_SAFETY */
! struct sigpipe_info {
! pqsigfunc oldhandler;
! };
! static inline int disable_sigpipe(struct sigpipe_info *info)
! {
! info-oldhandler = pqsignal(SIGPIPE, SIG_IGN);
! return 0;
! }
!
! static inline void remember_epipe(struct sigpipe_info *info, bool cond)
! {
! }
! static inline void restore_sigpipe(struct sigpipe_info *info)
! {
! pqsignal(SIGPIPE, info-oldhandler);
! }
#endif/* ENABLE_THREAD_SAFETY */
#else /* WIN32 */
! struct sigpipe_info { };
! static inline int disable_sigpipe(struct sigpipe_info *info) { return 0; }
! static inline void remember_epipe(struct sigpipe_info *info, bool cond) { }
! static inline void restore_sigpipe(struct sigpipe_info *info) { }
#endif/* WIN32 */
***
*** 286,294 pqsecure_read(PGconn *conn, void *ptr, size_t len)
if (conn-ssl)
{
int err;
/* SSL_read can write to the socket, so we need to disable
SIGPIPE */
! DISABLE_SIGPIPE(return -1);
rloop:
n = SSL_read(conn-ssl, ptr, len);
--- 303,313
if (conn-ssl)
{
int err;
+ struct sigpipe_info info;
/* SSL_read can write to the socket, so we need to disable
SIGPIPE */
! if (disable_sigpipe(info))
! return -1;
rloop:
n = SSL_read(conn-ssl, ptr, len);
***
*** 315,321 rloop:
if (n == -1)
{
! REMEMBER_EPIPE(SOCK_ERRNO ==
EPIPE);
printfPQExpBuffer(conn-errorMessage,
libpq_gettext(SSL SYSCALL error: %s\n),
SOCK_STRERROR(SOCK_ERRNO, sebuf,
