btrfs-progs: fix clone from wrong subvolume
cover btrfs-progs: fix clone from wrong subvolume
From: Omar Sandoval <[email protected]>
Hi,
This series is a couple of minor fixes to btrfs receive. Patch 1 fixes a missing strdup() return value check by getting rid of the strdup(). Patch 2 (the more important one) fixes a receive error when the same subvolume is received multiple times.
Thanks,
Omar
Omar Sandoval (2):
btrfs-progs: receive: get rid of unnecessary strdup()
btrfs-progs: receive: don't lookup clone root for received subvolume
cmds/receive.c | 41 +++++++++++------------------------------ 1 file changed, 11 insertions(+), 30 deletions(-)
Signature · 1 line
2.22.0
PATCH 1/2 btrfs-progs: receive: get rid of unnecessary strdup()
From: Omar Sandoval <[email protected]>
In process_clone(), we're not checking the return value of strdup(). But, there's no reason to strdup() in the first place: we just pass the path into path_cat_out(). Get rid of the strdup().
--- cmds/receive.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
cmds/receive.c +4 -5
@@ -739,7 +739,7 @@ static int process_clone(const char *path, u64 offset, u64 len,struct btrfs_ioctl_clone_range_args clone_args;struct subvol_info *si =NULL ;char full_path[PATH_MAX ]; -char *subvol_path =NULL ; +char *subvol_path;char full_clone_path[PATH_MAX ];int clone_fd =-1 ;
@@ -760,7 +760,7 @@ static int process_clone(const char *path, u64 offset, u64 len,if (memcmp (clone_uuid, rctx->cur_subvol.received_uuid,BTRFS_UUID_SIZE ) ==0 ) {/* TODO check generation of extent */ - subvol_path =strdup (rctx->cur_subvol_path); + subvol_path = rctx->cur_subvol_path; }else {if (!si) ret = -ENOENT ;
@@ -794,14 +794,14 @@ static int process_clone(const char *path, u64 offset, u64 len,if (sub_len > root_len &&strstr (si->path, rctx->full_root_path) == si->path && si->path[root_len] =='/' ) { - subvol_path =strdup (si->path + root_len +1 ); + subvol_path = si->path + root_len +1 ; }else {error ("clone: source subvol path %s unreachable from %s" , si->path, rctx->full_root_path); goto out; } }else { - subvol_path =strdup (si->path); + subvol_path = si->path; } }
@@ -839,7 +839,6 @@ out:free (si->path);free (si); } -free (subvol_path);if (clone_fd !=-1 )close (clone_fd);return ret;
Signature · 1 line
2.22.0
PATCH 2/2 btrfs-progs: receive: don't lookup clone root for received subvolume
In reply to Omar SandovalFrom: Omar Sandoval <[email protected]>
When we process a clone request, we look up the source subvolume by UUID, even if the source is the subvolume that we're currently receiving. Usually, this is fine. However, if for some reason we previously received the same subvolume, then this will use paths relative to the previously received subvolume instead of the current one. This is incorrect, since the send stream may use temporary names for the clone source. This can be reproduced as follows:
btrfs subvol create subvol
dd if=/dev/urandom of=subvol/foo bs=1M count=1
cp --reflink subvol/foo subvol/bar
mkdir subvol/dir
mv subvol/foo subvol/dir/
btrfs property set subvol ro true
btrfs send -f stream subvol
mkdir first second
btrfs receive -f stream first
btrfs receive -f stream second
The second receive results in this error:
ERROR: cannot open first/subvol/o259-7-0/foo: No such file or directory
Fix it by always cloning from the current subvolume if its UUID matches. This has the nice side effect of avoiding unnecessary UUID tree lookups in that case. Also, while we're here, get rid of some code that has been commented out since it was added.
--- cmds/receive.c | 34 ++++++++-------------------------- 1 file changed, 8 insertions(+), 26 deletions(-)
cmds/receive.c +8 -26
@@ -753,15 +753,14 @@ static int process_clone(const char *path, u64 offset, u64 len,if (ret <0 ) goto out; - si =subvol_uuid_search (&rctx->sus,0 , clone_uuid, clone_ctransid, -NULL , - subvol_search_by_received_uuid); -if (IS_ERR_OR_NULL (si)) { -if (memcmp (clone_uuid, rctx->cur_subvol.received_uuid, -BTRFS_UUID_SIZE ) ==0 ) { -/* TODO check generation of extent */ - subvol_path = rctx->cur_subvol_path; - }else { +if (memcmp (clone_uuid, rctx->cur_subvol.received_uuid, +BTRFS_UUID_SIZE ) ==0 ) { + subvol_path = rctx->cur_subvol_path; + }else { + si =subvol_uuid_search (&rctx->sus,0 , clone_uuid, clone_ctransid, +NULL , + subvol_search_by_received_uuid); +if (IS_ERR_OR_NULL (si)) {if (!si) ret = -ENOENT ;else
@@ -769,23 +768,6 @@ static int process_clone(const char *path, u64 offset, u64 len,error ("clone: did not find source subvol" ); goto out; } - }else { -/*if (rs_args.ctransid > rs_args.rtransid) { -if (!r->force) { -ret = -EINVAL; -fprintf(stderr, "ERROR: subvolume %s was " -"modified after it was " -"received.\n", -r->subvol_parent_name); -goto out; -} else { -fprintf(stderr, "WARNING: subvolume %s was " -"modified after it was " -"received.\n", -r->subvol_parent_name); -} -}*/ -/* strip the subvolume that we are receiving to from the start of subvol_path */ if (rctx->full_root_path) {size_t root_len =strlen (rctx->full_root_path);
Signature · 1 line
2.22.0
Re: [PATCH 1/2] btrfs-progs: receive: get rid of unnecessary strdup()
In reply to Omar SandovalQuoted 8 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
In
process_clone(), we're not checking the return value ofstrdup(). But, there's no reason tostrdup()in the first place: we just pass the path intopath_cat_out(). Get rid of thestrdup().Fixes f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands")Signed-off-by Omar Sandoval <[email protected]>
Wrong SOB on this one, should be the usual
Re: [PATCH 1/2] btrfs-progs: receive: get rid of unnecessary strdup()
In reply to Omar SandovalQuoted 18 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
In
process_clone(), we're not checking the return value ofstrdup(). But, there's no reason tostrdup()in the first place: we just pass the path intopath_cat_out(). Get rid of thestrdup().Fixes f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands")Signed-off-by Omar Sandoval <[email protected]>--- cmds/receive.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)cmds/receive.c +0
-0
cmds/receive.c static int process_clone(const char *path, u64 offset, u64 len, · line 742
Quoted 5 lines
cmds/receive.c +1
-1@@ -739,7 +739,7 @@ static int process_clone(const char *path, u64 offset, u64 len,struct btrfs_ioctl_clone_range_args clone_args;struct subvol_info *si =NULL ;char full_path[PATH_MAX ]; -char *subvol_path =NULL ; +char *subvol_path;
I think that should become const char *.
Quoted 38 lines
cmds/receive.c +3
-5char full_clone_path[PATH_MAX ];int clone_fd =-1 ;@@ -760,7 +760,7 @@ static int process_clone(const char *path, u64 offset, u64 len,if (memcmp (clone_uuid, rctx->cur_subvol.received_uuid,BTRFS_UUID_SIZE ) ==0 ) {/* TODO check generation of extent */ - subvol_path =strdup (rctx->cur_subvol_path); + subvol_path = rctx->cur_subvol_path; }else {if (!si) ret = -ENOENT ;@@ -794,14 +794,14 @@ static int process_clone(const char *path, u64 offset, u64 len,if (sub_len > root_len &&strstr (si->path, rctx->full_root_path) == si->path && si->path[root_len] =='/' ) { - subvol_path =strdup (si->path + root_len +1 ); + subvol_path = si->path + root_len +1 ; }else {error ("clone: source subvol path %s unreachable from %s" , si->path, rctx->full_root_path); goto out; } }else { - subvol_path =strdup (si->path); + subvol_path = si->path; } }@@ -839,7 +839,6 @@ out:free (si->path);free (si); } -free (subvol_path);if (clone_fd !=-1 )close (clone_fd);return ret; --
Quoted 2 lines
2.22.0
Mike
Re: [PATCH 1/2] btrfs-progs: receive: get rid of unnecessary strdup()
Quoted 14 lines · Mike Fleetwood
Quoted 13 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
In
process_clone(), we're not checking the return value ofstrdup(). But, there's no reason tostrdup()in the first place: we just pass the path intopath_cat_out(). Get rid of thestrdup().Fixes f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands")Signed-off-by Omar Sandoval <[email protected]>--- cmds/receive.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
Quoted 11 lines
cmds/receive.c static int process_clone(const char *path, u64 offset, u64 len, · line 742
Quoted 10 lines
cmds/receive.c +1
-1@@ -739,7 +739,7 @@ static int process_clone(const char *path, u64 offset, u64 len,struct btrfs_ioctl_clone_range_args clone_args;struct subvol_info *si =NULL ;char full_path[PATH_MAX ]; -char *subvol_path =NULL ; +char *subvol_path;I think that should become const char *.
Yeah, that wouldn't hurt. Dave, can you add that when you apply this or should I resend?
Re: [PATCH 2/2] btrfs-progs: receive: don't lookup clone root for received subvolume
In reply to Omar SandovalQuoted 52 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
When we process a clone request, we look up the source subvolume by UUID, even if the source is the subvolume that we're currently receiving. Usually, this is fine. However, if for some reason we previously received the same subvolume, then this will use paths relative to the previously received subvolume instead of the current one. This is incorrect, since the send stream may use temporary names for the clone source. This can be reproduced as follows:
btrfs subvol create subvol
dd if=/dev/urandom of=subvol/foo bs=1M count=1
cp --reflink subvol/foo subvol/bar
mkdir subvol/dir
mv subvol/foo subvol/dir/
btrfs property set subvol ro true
btrfs send -f stream subvol
mkdir first second
btrfs receive -f stream first
btrfs receive -f stream secondThe second receive results in this error:
ERROR: cannot open first/subvol/o259-7-0/foo: No such file or directory
Fix it by always cloning from the current subvolume if its UUID matches. This has the nice side effect of avoiding unnecessary UUID tree lookups in that case. Also, while we're here, get rid of some code that has been commented out since it was added.
Fixes f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands")Signed-off-by Omar Sandoval <[email protected]>--- cmds/receive.c | 34 ++++++++-------------------------- 1 file changed, 8 insertions(+), 26 deletions(-)cmds/receive.c +0
-7@@ -753,15 +753,14 @@ static int process_clone(const char *path, u64 offset, u64 len,if (ret <0 ) goto out; - si =subvol_uuid_search (&rctx->sus,0 , clone_uuid, clone_ctransid, -NULL , - subvol_search_by_received_uuid); -if (IS_ERR_OR_NULL (si)) { -if (memcmp (clone_uuid, rctx->cur_subvol.received_uuid, -BTRFS_UUID_SIZE ) ==0 ) { -/* TODO check generation of extent */
cmds/receive.c static int process_clone(const char *path, u64 offset, u64 len, · line 763
Quoted 10 lines
cmds/receive.c +8
-2- subvol_path = rctx->cur_subvol_path; - }else { +if (memcmp (clone_uuid, rctx->cur_subvol.received_uuid, +BTRFS_UUID_SIZE ) ==0 ) { + subvol_path = rctx->cur_subvol_path; + }else { + si =subvol_uuid_search (&rctx->sus,0 , clone_uuid, clone_ctransid, +NULL , + subvol_search_by_received_uuid); +if (IS_ERR_OR_NULL (si)) {
Hi Omar,
This, and the change log, look good.
Quoted 14 lines
cmds/receive.c +0
-7if (!si) ret = -ENOENT ;else @@ -769,23 +768,6 @@ static int process_clone(const char *path, u64 offset, u64 len,error ("clone: did not find source subvol" ); goto out; } - }else { -/*if (rs_args.ctransid > rs_args.rtransid) { -if (!r->force) { -ret = -EINVAL; -fprintf(stderr, "ERROR: subvolume %s was " -"modified after it was " -"received.\n",
cmds/receive.c static int process_clone(const char *path, u64 offset, u64 len, · line 788
Quoted 10 lines
cmds/receive.c +0
-10-r->subvol_parent_name); -goto out; -} else { -fprintf(stderr, "WARNING: subvolume %s was " -"modified after it was " -"received.\n", -r->subvol_parent_name); -} -}*/ -
Isn't this unrelated? Shouldn't go to a separate patch?
Also, would you please create a test case as well?
Thanks.
Quoted 6 lines
cmds/receive.c +0
-1/* strip the subvolume that we are receiving to from the start of subvol_path */ if (rctx->full_root_path) {size_t root_len =strlen (rctx->full_root_path); --2.22.0
Signature · 3 lines
Filipe David Manana, “Whether you think you can, or you think you can't — you're right.”
Re: [PATCH 2/2] btrfs-progs: receive: don't lookup clone root for received subvolume
Quoted 86 lines · Filipe Manana
Quoted 52 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
When we process a clone request, we look up the source subvolume by UUID, even if the source is the subvolume that we're currently receiving. Usually, this is fine. However, if for some reason we previously received the same subvolume, then this will use paths relative to the previously received subvolume instead of the current one. This is incorrect, since the send stream may use temporary names for the clone source. This can be reproduced as follows:
btrfs subvol create subvol
dd if=/dev/urandom of=subvol/foo bs=1M count=1
cp --reflink subvol/foo subvol/bar
mkdir subvol/dir
mv subvol/foo subvol/dir/
btrfs property set subvol ro true
btrfs send -f stream subvol
mkdir first second
btrfs receive -f stream first
btrfs receive -f stream secondThe second receive results in this error:
ERROR: cannot open first/subvol/o259-7-0/foo: No such file or directory
Fix it by always cloning from the current subvolume if its UUID matches. This has the nice side effect of avoiding unnecessary UUID tree lookups in that case. Also, while we're here, get rid of some code that has been commented out since it was added.
Fixes f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands")Signed-off-by Omar Sandoval <[email protected]>--- cmds/receive.c | 34 ++++++++-------------------------- 1 file changed, 8 insertions(+), 26 deletions(-)cmds/receive.c +0
-7@@ -753,15 +753,14 @@ static int process_clone(const char *path, u64 offset, u64 len,if (ret <0 ) goto out; - si =subvol_uuid_search (&rctx->sus,0 , clone_uuid, clone_ctransid, -NULL , - subvol_search_by_received_uuid); -if (IS_ERR_OR_NULL (si)) { -if (memcmp (clone_uuid, rctx->cur_subvol.received_uuid, -BTRFS_UUID_SIZE ) ==0 ) { -/* TODO check generation of extent */ cmds/receive.c static int process_clone(const char *path, u64 offset, u64 len, · line 763
Quoted 10 lines
cmds/receive.c +8
-2- subvol_path = rctx->cur_subvol_path; - }else { +if (memcmp (clone_uuid, rctx->cur_subvol.received_uuid, +BTRFS_UUID_SIZE ) ==0 ) { + subvol_path = rctx->cur_subvol_path; + }else { + si =subvol_uuid_search (&rctx->sus,0 , clone_uuid, clone_ctransid, +NULL , + subvol_search_by_received_uuid); +if (IS_ERR_OR_NULL (si)) {Hi Omar,
This, and the change log, look good.
Quoted 18 lines
cmds/receive.c +0
-11if (!si) ret = -ENOENT ;else @@ -769,23 +768,6 @@ static int process_clone(const char *path, u64 offset, u64 len,error ("clone: did not find source subvol" ); goto out; } - }else { -/*if (rs_args.ctransid > rs_args.rtransid) { -if (!r->force) { -ret = -EINVAL; -fprintf(stderr, "ERROR: subvolume %s was " -"modified after it was " -"received.\n", -r->subvol_parent_name); -goto out; -} else { -fprintf(stderr, "WARNING: subvolume %s was "
Quoted 8 lines
cmds/receive.c static int process_clone(const char *path, u64 offset, u64 len, · line 788
Quoted 6 lines
cmds/receive.c +0
-6-"modified after it was " -"received.\n", -r->subvol_parent_name); -} -}*/ -Isn't this unrelated? Shouldn't go to a separate patch?
It didn't seem worth it to make a separate patch when I'm moving this code around anyways, but I noticed that there's a similar check in another location, so I'll just make it a separate patch.
Quoted 1 line
Also, would you please create a test case as well?
Will do.
Thanks!