Re: [HACKERS][PATCH] adding simple sock check for windows

From: CharSyam <charsyam(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [HACKERS][PATCH] adding simple sock check for windows
Date: 2018-03-31 17:37:28
Message-ID: CAMrLSE72q0ca2+Tf3V7-74NB-TeuSWh+0gLE5xdpEAxFt7JphQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi, Tom, Thank you for your review.
so Do you think it is better to remove if statement like below

diff --git src/bin/scripts/vacuumdb.c src/bin/scripts/vacuumdb.c
index 887fa48fbd..243d842d06 100644
--- src/bin/scripts/vacuumdb.c
+++ src/bin/scripts/vacuumdb.c
@@ -947,13 +947,6 @@ init_slot(ParallelSlot *slot, PGconn *conn, const
char *progname)
slot->connection = conn;
slot->isFree = true;
slot->sock = PQsocket(conn);
-
- if (slot->sock < 0)
- {
- fprintf(stderr, _("%s: invalid socket: %s"), progname,
- PQerrorMessage(conn));
- exit(1);
- }
}

static void

2018-04-01 2:16 GMT+09:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> CharSyam <charsyam(at)gmail(dot)com> writes:
>> [ simple_check.patch ]
>
> This is a good catch. However, it looks to me like the reason nobody
> has noticed a problem here is that actually, this error test is useless;
> we can never get here with a bad connection, because connectDatabase
> would have failed. I'm inclined to think we should just delete the whole
> if-statement, instead.
>
> BTW, I tried cross-checking for other errors of this ilk by doing
>
> diff --git a/src/include/port.h b/src/include/port.h
> index a514ab758b..9ba6a0df79 100644
> --- a/src/include/port.h
> +++ b/src/include/port.h
> @@ -28,9 +28,9 @@
>
> /* socket has a different definition on WIN32 */
> #ifndef WIN32
> -typedef int pgsocket;
> +typedef unsigned int pgsocket;
>
> -#define PGINVALID_SOCKET (-1)
> +#define PGINVALID_SOCKET ((pgsocket) -1)
> #else
> typedef SOCKET pgsocket;
>
> and then compiling with a compiler that would warn about "unsigned int < 0"
> tests (I used Apple's current compiler, but probably any recent clang
> would do as well). It found this case and no others, which is good.
> This is not a 100% cross-check, because I don't think it would notice
> "unsigned int == -1" which is another way somebody might misspell the
> test, and it definitely would not catch cases where somebody stored a
> socket identifier in plain int instead of pgsocket. But it's better
> than nothing...
>
> regards, tom lane

Attachment Content-Type Size
remove_if_stat.patch application/octet-stream 479 bytes

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Pavel Stehule 2018-03-31 17:38:42 some last patches breaks plan cache
Previous Message Tom Lane 2018-03-31 17:16:07 Re: [HACKERS][PATCH] adding simple sock check for windows