[pgpool-hackers: 4501] Re: Watchdog and IPv6

[Tatsuo Ishii]

> oh my..... I had attached wrong one.....
> 
> The old one have one compile warning in line at watchdog:3459 ereport(LOG, ...) and it fails also regression tests.
> 
> sorry for the inconvenience.
> 
> Before re-submit this patch file, I just checked regression tests, compile warning again.
> 
> Could you please discard submitted before and consider this as a actual one?
> 
> why I have realized so late! sorry again.

I think you should give a version number to your patch using "-v"
option of git format-patch command.

I have quick looked through the patch.


t-ishii$ git apply ~/0001-feature-ipv6-for-watchdog.patch 
/home/t-ishii/0001-feature-ipv6-for-watchdog.patch:343: trailing whitespace.
	
/home/t-ishii/0001-feature-ipv6-for-watchdog.patch:355: trailing whitespace.
	
warning: 2 lines add whitespace errors.

Please remove unnecessary white spaces.

Also I got compiler warnings:

In file included from watchdog.c:51:
watchdog.c: In function ‘watchdog_main’:
../../src/include/utils/elog.h:199:4: warning: ‘port’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  199 |    errfinish rest; \
      |    ^~~~~~~~~
watchdog.c:3439:8: note: ‘port’ was declared here
 3439 |    int port;
      |        ^~~~

This is caused by following code snipet:

+				switch(ss.ss_family)
+				{
+					case AF_INET:
+					inet_ntop(AF_INET, &((struct sockaddr_in*) &ss)->sin_addr, conn->addr, addrlen);
+					port = ntohs(((struct sockaddr_in*)(&ss))->sin_port);
+					break;
+
+					case AF_INET6:
+					inet_ntop(AF_INET6, &((struct sockaddr_in6*) &ss)->sin6_addr, conn->addr, addrlen);
+					port = ntohs(((struct sockaddr_in6*)(&ss))->sin6_port);
+					break;
+				}
+
+				ereport(LOG,
+						(errmsg("new watchdog node connection is received from \"%s:%d\"",conn->addr, port)));
+

Here the switch statement does not have "default" and if ss.ss_family
is not either AF_INET or AF_INET6, the value of "port" will be
undefined. That's why the compiler complains. I think you should have
"default" something like:

+					case AF_INET6:
+					inet_ntop(AF_INET6, &((struct sockaddr_in6*) &ss)->sin6_addr, conn->addr, addrlen);
+					port = ntohs(((struct sockaddr_in6*)(&ss))->sin6_port);
+					break;
+
					default:
						ereport(ERROR,
							(errmsg("unknown ss_family")));
						break;

BTW the indentations after CASE should be added. See other CASE
statements in pgpool.

+	else if (sel_ret == -1)
+	{
+		ereport(ERROR,
+				(errmsg("failed to get socket data from socket list")
+				 ,errdetail("select() failed with errno %d", errno)));

You can use %m" here without using errno. See codes using %m in pgpool.

+	}
+	else
+	{
+		/* unreachable. */
+		Assert(FALSE);
+	}

Assert cannot be used in pgpool for now.

Regarding the regression test, I got a timeout error with
028.watchdog_enable_consensus_with_half_votes test. Have you
succeeded?

Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp