Merge lp:~csurbhi/ubuntu/maverick/btrfs-tools/btrfs-tools.fix-601874 into lp:ubuntu/maverick/btrfs-tools

Proposed by Surbhi Palande
Status: Needs review
Proposed branch: lp:~csurbhi/ubuntu/maverick/btrfs-tools/btrfs-tools.fix-601874
Merge into: lp:ubuntu/maverick/btrfs-tools
Diff against target: 44 lines (+23/-1)
3 files modified
debian/changelog (+8/-1)
debian/patches/04-fix-601874.patch (+14/-0)
debian/patches/series (+1/-0)
To merge this branch: bzr merge lp:~csurbhi/ubuntu/maverick/btrfs-tools/btrfs-tools.fix-601874
Reviewer Review Type Date Requested Status
Colin Watson Needs Fixing
Review via email: mp+31722@code.launchpad.net

Description of the change

btrfs_search_slot() returns
* -1 when some error is encountered
* +1 when the item to be searched is not found
* 0 when the item is found successfully.

When read_node_slot() fails, this can only happen because of an I/O error or because malloc failed. Since the item to be searched is actually found, we should return a -1 and not +1 as it was originally returning. The caller expected that on +1, the path[] was appropriately populated and due to the error, it was not. Hence accessing the unpopulated path[] was segfaulting.
Do consider merging this for Maverick.

To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) wrote :

Could you send this upstream for comment, and link to the relevant thread? There are a few cases I was wondering about, such as:

 * Is it an I/O error if the slot passed to read_node_slot is out of range?
 * Are all the cases where we run off the end of the while (1) loop in read_tree_block I/O errors? I can't quite puzzle that out.
 * The caller only seems to care that p->nodes[0] is populated; what if read_node_slot fails at a deeper level?

I agree with you that p->nodes[0] isn't being properly populated, but since this needs to go through upstream anyway it would be good to confirm that this is the right way to handle this error.

A couple of packaging nits while I'm here:

 * The version number should be -4ubuntu1 for merging into maverick (though this is easy to fix up at sponsorship time).
 * Please make sure that new Ubuntu patches are pushed at the end of the patch queue, rather than at the start. That is, 'quilt push -a' before you run 'quilt new', and since this is source format 1.0 you'll need to also 'quilt pop -a' before committing. Since you've already created the patch, in this case it should suffice to pop all patches, edit debian/patches/series to move the new patch to the end, and then push up to the new patch, refresh, and pop back down.

Thanks!

review: Needs Fixing

Unmerged revisions

25. By Surbhi Palande

Returns -1 when a block cannot be read from disk. Closes (LP: #601874) and
(LP: #601877)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/changelog'
2--- debian/changelog 2010-06-20 21:22:57 +0000
3+++ debian/changelog 2010-08-04 08:45:54 +0000
4@@ -1,4 +1,11 @@
5-btrfs-tools (0.19+20100601-3) unstable; urgency=low
6+btrfs-tools (0.19+20100601-4ubuntu0+2) maverick; urgency=low
7+
8+ * Returns -1 when a block cannot be read from disk. Closes (LP: #601874) and
9+ (LP: #601877)
10+
11+ -- Surbhi Palande <surbhi.palande@canonical.com> Tue, 27 Jul 2010 19:45:00 +0300
12+
13+btrfs-tools (0.19+20100601-4) unstable; urgency=low
14
15 * Updating year in copyright file.
16 * Adding overrides for clean target to make sure package builds twice
17
18=== added file 'debian/patches/04-fix-601874.patch'
19--- debian/patches/04-fix-601874.patch 1970-01-01 00:00:00 +0000
20+++ debian/patches/04-fix-601874.patch 2010-08-04 08:45:54 +0000
21@@ -0,0 +1,14 @@
22+Index: b/ctree.c
23+===================================================================
24+--- a/ctree.c
25++++ b/ctree.c
26+@@ -1264,6 +1264,9 @@
27+ key->objectid);
28+
29+ b = read_node_slot(root, b, slot);
30++ if (!b) {
31++ return -1;
32++ }
33+ } else {
34+ p->slots[level] = slot;
35+ if (ins_len > 0 &&
36
37=== modified file 'debian/patches/series'
38--- debian/patches/series 2010-06-20 21:22:57 +0000
39+++ debian/patches/series 2010-08-04 08:45:54 +0000
40@@ -1,3 +1,4 @@
41+04-fix-601874.patch
42 01-labels.patch
43 02-ftbfs.patch
44 03-glibc.patch

Subscribers

People subscribed via source and target branches

to all changes: