[PATCH 0/2 v2] btrfs-progs: fix btrfs send & receive with -e flag

[PATCH 0/2 v2] btrfs-progs: fix btrfs send & receive with -e flag

[Christian Brauner]

Hi guys,

This is the second version of the patch. It contains no functional changes but
merely adds a second small patch to adapt the test to use -e with btrfs receive
in order to terminate on an end marker in the stream. Thanks to David for
pointing this out. Here's the description of the patch again:

The old check here tried to ensure that empty streams are not considered valid.
The old check however, will always fail when only one run through the while(1)
loop is needed and honor_end_cmd is set. So this:

btrfs send /some/subvol | btrfs receive -e /some/

will consistently fail because -e causes honor_cmd_to be set and
btrfs_read_and_process_send_stream() to correctly return 1. So the command will
be successful but btrfs receive will error out because the send - receive
concluded in one run through the while(1) loop.

If we want to exclude empty streams we need a way to tell the difference between
btrfs_read_and_process_send_stream() returning 1 because read_buf() did not
detect any data and read_and_process_cmd() returning 1 because honor_end_cmd was
set. Without introducing too many changes the best way to me seems to have
btrfs_read_and_process_send_stream() return -ENODATA in the first case. The rest
stays the same. We can then check for -ENODATA in do_receive() and report a
proper error in this case. This should also be backwards compatible to previous
versions of btrfs receive. They will fail on empty streams because a negative
value is returned. The only thing that they will lack is a nice error message.

Christian Brauner (2):
  btrfs-progs: fix btrfs send & receive with -e flag
  tests: use receive -e to terminate on end marker

 cmds-receive.c                                  | 13 +++++--------
 send-stream.c                                   |  2 +-
 tests/misc-tests/018-recv-end-of-stream/test.sh | 12 ++++++------
 3 files changed, 12 insertions(+), 15 deletions(-)

-- 
2.11.0

[PATCH 1/2 v2] btrfs-progs: fix btrfs send & receive with -e flag

[Christian Brauner]

The old check here tried to ensure that empty streams are not considered valid.
The old check however, will always fail when only one run through the while(1)
loop is needed and honor_end_cmd is set. So this:

btrfs send /some/subvol | btrfs receive -e /some/

will consistently fail because -e causes honor_cmd_to be set and
btrfs_read_and_process_send_stream() to correctly return 1. So the command will
be successful but btrfs receive will error out because the send - receive
concluded in one run through the while(1) loop.

If we want to exclude empty streams we need a way to tell the difference between
btrfs_read_and_process_send_stream() returning 1 because read_buf() did not
detect any data and read_and_process_cmd() returning 1 because honor_end_cmd was
set. Without introducing too many changes the best way to me seems to have
btrfs_read_and_process_send_stream() return -ENODATA in the first case. The rest
stays the same. We can then check for -ENODATA in do_receive() and report a
proper error in this case. This should also be backwards compatible to previous
versions of btrfs receive. They will fail on empty streams because a negative
value is returned. The only thing that they will lack is a nice error message.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
Changelog: 2017-04-03
	- no changes
---
 cmds-receive.c | 13 +++++--------
 send-stream.c  |  2 +-
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/cmds-receive.c b/cmds-receive.c
index 6cf22637..b59f00e4 100644
--- a/cmds-receive.c
+++ b/cmds-receive.c
@@ -1091,7 +1091,6 @@ static int do_receive(struct btrfs_receive *rctx, const char *tomnt,
 	char *dest_dir_full_path;
 	char root_subvol_path[PATH_MAX];
 	int end = 0;
-	int count;
 
 	dest_dir_full_path = realpath(tomnt, NULL);
 	if (!dest_dir_full_path) {
@@ -1186,7 +1185,6 @@ static int do_receive(struct btrfs_receive *rctx, const char *tomnt,
 	if (ret < 0)
 		goto out;
 
-	count = 0;
 	while (!end) {
 		if (rctx->cached_capabilities_len) {
 			if (g_verbose >= 3)
@@ -1200,16 +1198,15 @@ static int do_receive(struct btrfs_receive *rctx, const char *tomnt,
 							 rctx,
 							 rctx->honor_end_cmd,
 							 max_errors);
-		if (ret < 0)
-			goto out;
-		/* Empty stream is invalid */
-		if (ret && count == 0) {
+		if (ret < 0 && ret == -ENODATA) {
+			/* Empty stream is invalid */
 			error("empty stream is not considered valid");
 			ret = -EINVAL;
 			goto out;
+		} else if (ret < 0) {
+			goto out;
 		}
-		count++;
-		if (ret)
+		if (ret > 0)
 			end = 1;
 
 		close_inode_for_write(rctx);
diff --git a/send-stream.c b/send-stream.c
index 5a028cd9..78f2571a 100644
--- a/send-stream.c
+++ b/send-stream.c
@@ -492,7 +492,7 @@ int btrfs_read_and_process_send_stream(int fd,
 	if (ret < 0)
 		goto out;
 	if (ret) {
-		ret = 1;
+		ret = -ENODATA;
 		goto out;
 	}
 
-- 
2.11.0

[PATCH 2/2 v2] tests: use receive -e to terminate on end marker

[Christian Brauner]

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 tests/misc-tests/018-recv-end-of-stream/test.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/misc-tests/018-recv-end-of-stream/test.sh b/tests/misc-tests/018-recv-end-of-stream/test.sh
index d39683e9..90655929 100755
--- a/tests/misc-tests/018-recv-end-of-stream/test.sh
+++ b/tests/misc-tests/018-recv-end-of-stream/test.sh
@@ -34,7 +34,7 @@ test_full_empty_stream() {
 
 	run_check $TOP/mkfs.btrfs -f $TEST_DEV
 	run_check_mount_test_dev
-	run_check $SUDO_HELPER $TOP/btrfs receive -v -f "$str" "$TEST_MNT"
+	run_check $SUDO_HELPER $TOP/btrfs receive -e -v -f "$str" "$TEST_MNT"
 	run_check_umount_test_dev
 
 	run_check rm -f -- "$str"
@@ -65,7 +65,7 @@ test_full_simple_stream() {
 
 	run_check $TOP/mkfs.btrfs -f $TEST_DEV
 	run_check_mount_test_dev
-	run_check $SUDO_HELPER $TOP/btrfs receive -v -f "$str" "$TEST_MNT"
+	run_check $SUDO_HELPER $TOP/btrfs receive -e -v -f "$str" "$TEST_MNT"
 	run_check_umount_test_dev
 
 	run_check rm -f -- "$str"
@@ -96,8 +96,8 @@ test_incr_empty_stream() {
 
 	run_check $TOP/mkfs.btrfs -f $TEST_DEV
 	run_check_mount_test_dev
-	run_check $SUDO_HELPER $TOP/btrfs receive -v -f "$fstr" "$TEST_MNT"
-	run_check $SUDO_HELPER $TOP/btrfs receive -v -f "$istr" "$TEST_MNT"
+	run_check $SUDO_HELPER $TOP/btrfs receive -e -v -f "$fstr" "$TEST_MNT"
+	run_check $SUDO_HELPER $TOP/btrfs receive -e -v -f "$istr" "$TEST_MNT"
 	run_check_umount_test_dev
 
 	run_check rm -f -- "$fstr" "$istr"
@@ -136,8 +136,8 @@ test_incr_simple_stream() {
 
 	run_check $TOP/mkfs.btrfs -f $TEST_DEV
 	run_check_mount_test_dev
-	run_check $SUDO_HELPER $TOP/btrfs receive -v -f "$fstr" "$TEST_MNT"
-	run_check $SUDO_HELPER $TOP/btrfs receive -v -f "$istr" "$TEST_MNT"
+	run_check $SUDO_HELPER $TOP/btrfs receive -e -v -f "$fstr" "$TEST_MNT"
+	run_check $SUDO_HELPER $TOP/btrfs receive -e -v -f "$istr" "$TEST_MNT"
 	run_check_umount_test_dev
 
 	run_check rm -f -- "$fstr" "$istr"
-- 
2.11.0

Re: [PATCH 2/2 v2] tests: use receive -e to terminate on end marker

[David Sterba]

On Mon, Apr 03, 2017 at 10:21:08PM +0200, Christian Brauner wrote:
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
>  tests/misc-tests/018-recv-end-of-stream/test.sh | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/misc-tests/018-recv-end-of-stream/test.sh b/tests/misc-tests/018-recv-end-of-stream/test.sh
> index d39683e9..90655929 100755
> --- a/tests/misc-tests/018-recv-end-of-stream/test.sh
> +++ b/tests/misc-tests/018-recv-end-of-stream/test.sh
> @@ -34,7 +34,7 @@ test_full_empty_stream() {
>  
>  	run_check $TOP/mkfs.btrfs -f $TEST_DEV
>  	run_check_mount_test_dev
> -	run_check $SUDO_HELPER $TOP/btrfs receive -v -f "$str" "$TEST_MNT"
> +	run_check $SUDO_HELPER $TOP/btrfs receive -e -v -f "$str" "$TEST_MNT"

You're changing an existing test, what I expect is a new testcase as the
existing tests cover existing usecases and must not break. However I
don't see now how to fix it, will look into it again next week.

Re: [PATCH 1/2 v2] btrfs-progs: fix btrfs send & receive with -e flag

[Christian Brauner]

Hi,

On Fri, Apr 28, 2017 at 02:55:31PM +0530, Lakshmipathi.G wrote:
> Seems like user reported an issue with this patch. please check
> https://bugzilla.kernel.org/show_bug.cgi?id=195597

I can take a look. What I'm wondering about is why it fails only in the HDD
to SSD case. If -ENODATA is returned with this patch it should mean that there
was no header data. So is the user sure that this doesn't indicate a valid
error?

Christian

> 
> ----
> Cheers,
> Lakshmipathi.G
> 
> 
> On Tue, Apr 4, 2017 at 1:51 AM, Christian Brauner <
> christian.brauner@ubuntu.com> wrote:
> > The old check here tried to ensure that empty streams are not considered
> valid.
> > The old check however, will always fail when only one run through the
> while(1)
> > loop is needed and honor_end_cmd is set. So this:
> >
> > btrfs send /some/subvol | btrfs receive -e /some/
> >
> > will consistently fail because -e causes honor_cmd_to be set and
> > btrfs_read_and_process_send_stream() to correctly return 1. So the
> command will
> > be successful but btrfs receive will error out because the send - receive
> > concluded in one run through the while(1) loop.
> >
> > If we want to exclude empty streams we need a way to tell the difference
> between
> > btrfs_read_and_process_send_stream() returning 1 because read_buf() did
> not
> > detect any data and read_and_process_cmd() returning 1 because
> honor_end_cmd was
> > set. Without introducing too many changes the best way to me seems to have
> > btrfs_read_and_process_send_stream() return -ENODATA in the first case.
> The rest
> > stays the same. We can then check for -ENODATA in do_receive() and report
> a
> > proper error in this case. This should also be backwards compatible to
> previous
> > versions of btrfs receive. They will fail on empty streams because a
> negative
> > value is returned. The only thing that they will lack is a nice error
> message.
> >
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> > Changelog: 2017-04-03
> >         - no changes
> > ---
> >  cmds-receive.c | 13 +++++--------
> >  send-stream.c  |  2 +-
> >  2 files changed, 6 insertions(+), 9 deletions(-)
> >
> > diff --git a/cmds-receive.c b/cmds-receive.c
> > index 6cf22637..b59f00e4 100644
> > --- a/cmds-receive.c
> > +++ b/cmds-receive.c
> > @@ -1091,7 +1091,6 @@ static int do_receive(struct btrfs_receive *rctx,
> const char *tomnt,
> >         char *dest_dir_full_path;
> >         char root_subvol_path[PATH_MAX];
> >         int end = 0;
> > -       int count;
> >
> >         dest_dir_full_path = realpath(tomnt, NULL);
> >         if (!dest_dir_full_path) {
> > @@ -1186,7 +1185,6 @@ static int do_receive(struct btrfs_receive *rctx,
> const char *tomnt,
> >         if (ret < 0)
> >                 goto out;
> >
> > -       count = 0;
> >         while (!end) {
> >                 if (rctx->cached_capabilities_len) {
> >                         if (g_verbose >= 3)
> > @@ -1200,16 +1198,15 @@ static int do_receive(struct btrfs_receive *rctx,
> const char *tomnt,
> >                                                          rctx,
> >
>  rctx->honor_end_cmd,
> >                                                          max_errors);
> > -               if (ret < 0)
> > -                       goto out;
> > -               /* Empty stream is invalid */
> > -               if (ret && count == 0) {
> > +               if (ret < 0 && ret == -ENODATA) {
> > +                       /* Empty stream is invalid */
> >                         error("empty stream is not considered valid");
> >                         ret = -EINVAL;
> >                         goto out;
> > +               } else if (ret < 0) {
> > +                       goto out;
> >                 }
> > -               count++;
> > -               if (ret)
> > +               if (ret > 0)
> >                         end = 1;
> >
> >                 close_inode_for_write(rctx);
> > diff --git a/send-stream.c b/send-stream.c
> > index 5a028cd9..78f2571a 100644
> > --- a/send-stream.c
> > +++ b/send-stream.c
> > @@ -492,7 +492,7 @@ int btrfs_read_and_process_send_stream(int fd,
> >         if (ret < 0)
> >                 goto out;
> >         if (ret) {
> > -               ret = 1;
> > +               ret = -ENODATA;
> >                 goto out;
> >         }
> >
> > --
> > 2.11.0
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2 v2] btrfs-progs: fix btrfs send & receive with -e flag

[Lakshmipathi.G]

Hi.

Adding the bug reporter, Nazar for the discussion (as I'm not familiar
with send/receive feature/code).

----
Cheers,
Lakshmipathi.G
http://www.giis.co.in http://www.webminal.org

On Fri, Apr 28, 2017 at 3:25 PM, Christian Brauner
<christian.brauner@canonical.com> wrote:
>
> Hi,
>
> On Fri, Apr 28, 2017 at 02:55:31PM +0530, Lakshmipathi.G wrote:
> > Seems like user reported an issue with this patch. please check
> > https://bugzilla.kernel.org/show_bug.cgi?id=195597
>
> I can take a look. What I'm wondering about is why it fails only in the HDD
> to SSD case. If -ENODATA is returned with this patch it should mean that there
> was no header data. So is the user sure that this doesn't indicate a valid
> error?
>
> Christian
>
> >
> > ----
> > Cheers,
> > Lakshmipathi.G
> >
> >
> > On Tue, Apr 4, 2017 at 1:51 AM, Christian Brauner <
> > christian.brauner@ubuntu.com> wrote:
> > > The old check here tried to ensure that empty streams are not considered
> > valid.
> > > The old check however, will always fail when only one run through the
> > while(1)
> > > loop is needed and honor_end_cmd is set. So this:
> > >
> > > btrfs send /some/subvol | btrfs receive -e /some/
> > >
> > > will consistently fail because -e causes honor_cmd_to be set and
> > > btrfs_read_and_process_send_stream() to correctly return 1. So the
> > command will
> > > be successful but btrfs receive will error out because the send - receive
> > > concluded in one run through the while(1) loop.
> > >
> > > If we want to exclude empty streams we need a way to tell the difference
> > between
> > > btrfs_read_and_process_send_stream() returning 1 because read_buf() did
> > not
> > > detect any data and read_and_process_cmd() returning 1 because
> > honor_end_cmd was
> > > set. Without introducing too many changes the best way to me seems to have
> > > btrfs_read_and_process_send_stream() return -ENODATA in the first case.
> > The rest
> > > stays the same. We can then check for -ENODATA in do_receive() and report
> > a
> > > proper error in this case. This should also be backwards compatible to
> > previous
> > > versions of btrfs receive. They will fail on empty streams because a
> > negative
> > > value is returned. The only thing that they will lack is a nice error
> > message.
> > >
> > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > ---
> > > Changelog: 2017-04-03
> > >         - no changes
> > > ---
> > >  cmds-receive.c | 13 +++++--------
> > >  send-stream.c  |  2 +-
> > >  2 files changed, 6 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/cmds-receive.c b/cmds-receive.c
> > > index 6cf22637..b59f00e4 100644
> > > --- a/cmds-receive.c
> > > +++ b/cmds-receive.c
> > > @@ -1091,7 +1091,6 @@ static int do_receive(struct btrfs_receive *rctx,
> > const char *tomnt,
> > >         char *dest_dir_full_path;
> > >         char root_subvol_path[PATH_MAX];
> > >         int end = 0;
> > > -       int count;
> > >
> > >         dest_dir_full_path = realpath(tomnt, NULL);
> > >         if (!dest_dir_full_path) {
> > > @@ -1186,7 +1185,6 @@ static int do_receive(struct btrfs_receive *rctx,
> > const char *tomnt,
> > >         if (ret < 0)
> > >                 goto out;
> > >
> > > -       count = 0;
> > >         while (!end) {
> > >                 if (rctx->cached_capabilities_len) {
> > >                         if (g_verbose >= 3)
> > > @@ -1200,16 +1198,15 @@ static int do_receive(struct btrfs_receive *rctx,
> > const char *tomnt,
> > >                                                          rctx,
> > >
> >  rctx->honor_end_cmd,
> > >                                                          max_errors);
> > > -               if (ret < 0)
> > > -                       goto out;
> > > -               /* Empty stream is invalid */
> > > -               if (ret && count == 0) {
> > > +               if (ret < 0 && ret == -ENODATA) {
> > > +                       /* Empty stream is invalid */
> > >                         error("empty stream is not considered valid");
> > >                         ret = -EINVAL;
> > >                         goto out;
> > > +               } else if (ret < 0) {
> > > +                       goto out;
> > >                 }
> > > -               count++;
> > > -               if (ret)
> > > +               if (ret > 0)
> > >                         end = 1;
> > >
> > >                 close_inode_for_write(rctx);
> > > diff --git a/send-stream.c b/send-stream.c
> > > index 5a028cd9..78f2571a 100644
> > > --- a/send-stream.c
> > > +++ b/send-stream.c
> > > @@ -492,7 +492,7 @@ int btrfs_read_and_process_send_stream(int fd,
> > >         if (ret < 0)
> > >                 goto out;
> > >         if (ret) {
> > > -               ret = 1;
> > > +               ret = -ENODATA;
> > >                 goto out;
> > >         }
> > >
> > > --
> > > 2.11.0
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 1/2 v2] btrfs-progs: fix btrfs send & receive with -e flag

[Nazar Mokrynskyi]

Hi,

Sorry for confusion, I've checked once again and the same issue happens in all cases.

I didn't notice this because my regular backups are done automatically in cron task + snapshots look fine despite the error, so I incorrectly assumed an error didn't happen there, but it actually did.

I've clarified this in last comment on bugzilla.

Sincerely, Nazar Mokrynskyi
github.com/nazar-pc
Tox: A9D95C9AA5F7A3ED75D83D0292E22ACE84BA40E912185939414475AF28FD2B2A5C8EF5261249

28.04.17 13:03, Lakshmipathi.G пише:
> I can take a look. What I'm wondering about is why it fails only in the HDD
> to SSD case. If -ENODATA is returned with this patch it should mean that there
> was no header data. So is the user sure that this doesn't indicate a valid
> error?
>
> Christian