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

[Tatsuo Ishii]

> 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?

In addition, 036.trusted_servers also timed out here.

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