Merge lp:~akopytov/percona-xtrabackup/bug1273207-2.1 into lp:percona-xtrabackup/2.1

[Alexey Kopytov]

    Bug #1273207: SIGPIPE handling of xbstream

    Replaced my_read(..., MYF(MY_FULL_IO)) with a new wrapper around
    my_read() which does the same, but bails out on EOF and errors returning
    the total number of successfully read bytes. This allows to detect a
    closed pipe and abort instead of waiting infinitely for more data.

http://jenkins.percona.com/view/PXB%202.1/job/percona-xtrabackup-2.1-param/551/

[Sergei Glushchenko (sergei.glushchenko)]

xb_crypt_read_callback returns ssize_t while xb_read_full returns size_t. Negative values do not make sense for both.
Is there any reason to conversion size_t -> ssize_t in xb_read_full?
xb_crypt_read_callback definition changed by this patch anyway, maybe would be better to return size_t from
xb_crypt_read_callback? It would also allow to get rid of a number of type conversions in xbcrypt_read.c

review: Needs Information (g2)

[Alexey Kopytov (akopytov)]

Right, xb_crypt_read_callback() does not have to return ssize_t. Recommitted.

http://jenkins.percona.com/view/PXB%202.1/job/percona-xtrabackup-2.1-param/554/

[Sergei Glushchenko (sergei.glushchenko)]

Approve

review: Approve (g2)

=== modified file 'src/common.h'
--- src/common.h	2013-08-18 06:54:14 +0000
+++ src/common.h	2014-02-28 14:37:08 +0000
@@ -89,4 +89,28 @@
     return (value >> 1) ? 0 : shift;
 }
 
+/****************************************************************************
+Read 'len' bytes from 'fd'. It is identical to my_read(..., MYF(MY_FULL_IO)),
+i.e. tries to combine partial reads into a single block of size 'len', except
+that it bails out on EOF or error, and returns the number of successfully read
+bytes instead. */
+static inline size_t
+xb_read_full(File fd, uchar *buf, size_t len)
+{
+	size_t tlen = 0;
+	size_t tbytes;
+
+	while (tlen < len) {
+		tbytes = my_read(fd, buf, len - tlen, MYF(MY_WME));
+		if (tbytes == 0 || tbytes == MY_FILE_ERROR) {
+			break;
+		}
+
+		buf += tbytes;
+		tlen += tbytes;
+	}
+
+	return tlen;
+}
+
 #endif

=== modified file 'src/xbcrypt.c'
--- src/xbcrypt.c	2013-11-27 18:43:59 +0000
+++ src/xbcrypt.c	2014-02-28 14:37:08 +0000
@@ -279,11 +279,11 @@
 
 
 static
-ssize_t
-my_xb_crypt_read_callback(void *userdata, void *buf, size_t len, int flags)
+size_t
+my_xb_crypt_read_callback(void *userdata, void *buf, size_t len)
 {
 	File* file = (File *) userdata;
-	return my_read(*file, buf, len, flags);
+	return xb_read_full(*file, buf, len);
 }
 
 static

=== modified file 'src/xbcrypt.h'
--- src/xbcrypt.h	2013-11-27 18:43:59 +0000
+++ src/xbcrypt.h	2014-02-28 14:37:08 +0000
@@ -53,8 +53,7 @@
 typedef struct xb_rcrypt_struct xb_rcrypt_t;
 
 /* Callback on read for i/o, must return # of bytes read or -1 on error */
-typedef ssize_t xb_crypt_read_callback(void *userdata,
-				       void *buf, size_t len, int flags);
+typedef size_t xb_crypt_read_callback(void *userdata, void *buf, size_t len);
 
 xb_rcrypt_t *xb_crypt_read_open(void *userdata,
 				  xb_crypt_read_callback *onread);

=== modified file 'src/xbcrypt_read.c'
--- src/xbcrypt_read.c	2013-10-17 01:00:57 +0000
+++ src/xbcrypt_read.c	2014-02-28 14:37:08 +0000
@@ -58,10 +58,10 @@
 	uchar		*ptr;
 	ulonglong	tmp;
 	ulong		checksum, checksum_exp, version;
-	ssize_t		bytesread;
+	size_t		bytesread;
 	xb_rcrypt_result_t result = XB_CRYPT_READ_CHUNK;
 
-	if ((bytesread = crypt->read(crypt->userdata, tmpbuf, sizeof(tmpbuf), MYF(MY_WME)))
+	if ((bytesread = crypt->read(crypt->userdata, tmpbuf, sizeof(tmpbuf)))
 	    != sizeof(tmpbuf)) {
 		if (bytesread == 0) {
 			result = XB_CRYPT_READ_EOF;
@@ -126,8 +126,8 @@
 		*ivlen = 0;
 		*iv = 0;
 	} else {
-		if ((bytesread = crypt->read(crypt->userdata, tmpbuf, 8,
-					     MYF(MY_WME))) != 8) {
+		if ((bytesread = crypt->read(crypt->userdata, tmpbuf, 8))
+		    != 8) {
 			if (bytesread == 0) {
 				result = XB_CRYPT_READ_EOF;
 				goto err;
@@ -176,8 +176,8 @@
 	}
 
 	if (*ivlen > 0) {
-		if (crypt->read(crypt->userdata, crypt->ivbuffer, *ivlen, MYF(MY_WME|MY_FULL_IO))
-		    != (ssize_t)*ivlen) {
+		if (crypt->read(crypt->userdata, crypt->ivbuffer, *ivlen)
+		    != *ivlen) {
 			msg("%s:%s: failed to read %lld bytes for chunk iv "
 			    "at offset 0x%llx.\n", my_progname, __FUNCTION__,
 			    (ulonglong)*ivlen, crypt->offset);
@@ -212,8 +212,8 @@
 	}
 
 	if (*elen > 0) {
-		if (crypt->read(crypt->userdata, crypt->buffer, *elen, MYF(MY_WME|MY_FULL_IO))
-		    != (ssize_t)*elen) {
+		if (crypt->read(crypt->userdata, crypt->buffer, *elen)
+		    != *elen) {
 			msg("%s:%s: failed to read %lld bytes for chunk payload "
 			    "at offset 0x%llx.\n", my_progname, __FUNCTION__,
 			    (ulonglong)*elen, crypt->offset);

=== modified file 'src/xbstream_read.c'
--- src/xbstream_read.c	2013-08-18 06:54:14 +0000
+++ src/xbstream_read.c	2014-02-28 14:37:08 +0000
@@ -73,8 +73,7 @@
 
 #define F_READ(buf,len)                                       	\
 	do {                                                      	\
-		if (my_read(fd, buf, len, MYF(MY_WME|MY_FULL_IO)) ==	\
-		    MY_FILE_ERROR) {					\
+		if (xb_read_full(fd, buf, len) < len) {	\
 			msg("xb_stream_read_chunk(): my_read() failed.\n"); \
 			goto err;                                 	\
 		}							\
@@ -90,7 +89,6 @@
 	uchar		tmpbuf[16];
 	uchar		*ptr = tmpbuf;
 	uint		pathlen;
-	size_t		tlen;
 	size_t		tbytes;
 	ulonglong	ullval;
 	ulong		checksum_exp;
@@ -99,20 +97,12 @@
 
 	xb_ad(sizeof(tmpbuf) >= CHUNK_HEADER_CONSTANT_LEN);
 
-	/* This is the only place where we expect EOF, so read with my_read()
-	rather than F_READ() */
-	tlen = CHUNK_HEADER_CONSTANT_LEN;
-	while (tlen > 0) {
-		tbytes = my_read(fd, ptr, tlen, MYF(MY_WME));
-		if (tbytes == 0) {
-			break;
-		}
-		ptr += tbytes;
-		tlen -= tbytes;
-	}
-	if (tlen == CHUNK_HEADER_CONSTANT_LEN) {
+	/* This is the only place where we expect EOF, so read with
+	xb_read_full() rather than F_READ() */
+	tbytes = xb_read_full(fd, ptr, CHUNK_HEADER_CONSTANT_LEN);
+	if (tbytes == 0) {
 		return XB_STREAM_READ_EOF;
-	} else if (tlen > 0) {
+	} else if (tbytes < CHUNK_HEADER_CONSTANT_LEN) {
 		msg("xb_stream_read_chunk(): unexpected end of stream at "
 		    "offset 0x%llx.\n", stream->offset);
 		goto err;

=== added file 'test/t/bug1273207.sh'
--- test/t/bug1273207.sh	1970-01-01 00:00:00 +0000
+++ test/t/bug1273207.sh	2014-02-28 14:37:08 +0000
@@ -0,0 +1,12 @@
+########################################################################
+# Bug #1273207: SIGPIPE handling of xbstream
+########################################################################
+
+# Create a truncated xbstream file containing only a valid chunk header
+echo "something" > $TEST_VAR_ROOT/file
+xbstream -c $TEST_VAR_ROOT/file | head -c 14 > $TEST_VAR_ROOT/file.xbstream
+
+# Write chunk header and then close the pipe. xbstream would hang before the fix
+cat $TEST_VAR_ROOT/file.xbstream | (xbstream -x || true)
+
+rm -f $TEST_VAR_ROOT/{file,file.xbstream}