subvolumes missing from "btrfs subvolume list" output

subvolumes missing from "btrfs subvolume list" output

[Stephane Chazelas]

Hiya,

I've got a btrfs FS with 84 subvolumes in it (some created
with "btrfs sub create", some with "btrfs sub snap" of the other
ones). There's no nesting of subvolumes at all (all direct
children of the root subvolume).

The "btrfs subvolume list" is only showing 80 subvolumes. The 4
missing ones (1 original volume, 3 snapshots) do exist on disk
and files in there have different st_devs from any other
subvolume.

How would I start investigating what's wrong? And how to fix it.

I found
http://thread.gmane.org/gmane.comp.file-systems.btrfs/8123/focus=8208

which looks like the same issue, with Li Zefan saying he had a
fix, but I couldn't find any mention that it was actually fixed.

Has anybody got any update on that?

Thanks in advance,
Stephane

Re: subvolumes missing from "btrfs subvolume list" output

[Stephane Chazelas]

2011-06-29 15:37:47 +0100, Stephane Chazelas:
[...]
> I found
> http://thread.gmane.org/gmane.comp.file-systems.btrfs/8123/focus=8208
> 
> which looks like the same issue, with Li Zefan saying he had a
> fix, but I couldn't find any mention that it was actually fixed.
> 
> Has anybody got any update on that?
[...]

I've found
http://thread.gmane.org/gmane.comp.file-systems.btrfs/8232

but no corresponding fix or ioctl.c
http://git.kernel.org/?p=linux/kernel/git/mason/btrfs-unstable.git;a=history;f=fs/btrfs/ioctl.c

I'm under the impression that the issue has been forgotten
about.

>From what I managed to gather though, it seems that what's on
disk is correct, it's just the ioctl and/or "btrfs sub list"
that's wrong. Am I right?

(btw, I forgot to mention the kernel version: 3.0rc4 amd64,
btrfs tools from git)

Cheers,
Stephane

Re: subvolumes missing from "btrfs subvolume list" output

[Josef Bacik]

On 06/29/2011 11:00 AM, Stephane Chazelas wrote:
> 2011-06-29 15:37:47 +0100, Stephane Chazelas:
> [...]
>> I found
>> http://thread.gmane.org/gmane.comp.file-systems.btrfs/8123/focus=8208
>>
>> which looks like the same issue, with Li Zefan saying he had a
>> fix, but I couldn't find any mention that it was actually fixed.
>>
>> Has anybody got any update on that?
> [...]
> 
> I've found
> http://thread.gmane.org/gmane.comp.file-systems.btrfs/8232
> 
> but no corresponding fix or ioctl.c
> http://git.kernel.org/?p=linux/kernel/git/mason/btrfs-unstable.git;a=history;f=fs/btrfs/ioctl.c
> 
> I'm under the impression that the issue has been forgotten
> about.
> 
> From what I managed to gather though, it seems that what's on
> disk is correct, it's just the ioctl and/or "btrfs sub list"
> that's wrong. Am I right?

Yeah, did you apply the patch from that thread and verify that it fixes
your problem?  Thanks,

Josef

Re: subvolumes missing from "btrfs subvolume list" output

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 3631 bytes --]

On Wed, Jun 29, 2011 at 12:16:06PM -0400, Josef Bacik wrote:
> On 06/29/2011 11:00 AM, Stephane Chazelas wrote:
> > 2011-06-29 15:37:47 +0100, Stephane Chazelas:
> > [...]
> >> I found
> >> http://thread.gmane.org/gmane.comp.file-systems.btrfs/8123/focus=8208
> >>
> >> which looks like the same issue, with Li Zefan saying he had a
> >> fix, but I couldn't find any mention that it was actually fixed.
> >>
> >> Has anybody got any update on that?
> > [...]
> > 
> > I've found
> > http://thread.gmane.org/gmane.comp.file-systems.btrfs/8232
> > 
> > but no corresponding fix or ioctl.c
> > http://git.kernel.org/?p=linux/kernel/git/mason/btrfs-unstable.git;a=history;f=fs/btrfs/ioctl.c
> > 
> > I'm under the impression that the issue has been forgotten
> > about.
> > 
> > From what I managed to gather though, it seems that what's on
> > disk is correct, it's just the ioctl and/or "btrfs sub list"
> > that's wrong. Am I right?
> 
> Yeah, did you apply the patch from that thread and verify that it fixes
> your problem?  Thanks,

   Note that changing this API will probably break btrfs-gui's listing
of subvolumes...

   The issue with that patch is that there are two distinct behaviours
that people want or expect with the tree-search ioctl:

(A) Return all items with keys which collate linearly between
    (min_objectid, min_type, min_offset) and 
    (max_objectid, max_type, max_offset)

    i.e. treating keys as indivisible objects and sorting lexically,
    as the trees do.

(B) Return all items with keys (i, t, o) which fulfil the criteria
    (min_objectid <= i <= max_objectid,
     min_type <= t <= max_type,
     min_offset <= o <= max_offset)

    i.e. treating keys as 3-tuples, and selecting from a rectilinear
    subsset of the tuple space, which is natural for some
    applications.

   Clearly, we can't do both with the same call (except for some
limited cases (*)). However, different users expect different
behaviours. The current behaviour is (A), which is the "natural"
behaviour for tree searches within the btrfs code, and is (IMO) the
right thing to be doing for an API like this.

   It sounds to me like the user of the API needs to be fixed, not the
ioctl itself -- possibly the author of the subvol scanning code
assumed (B) when they were getting (A). Note that there is at least
one other user of the ioctl outside btrfs-progs: btrfs-gui, which uses
the ioctl for several things, one of which is enumerating subvolumes
as btrfs-progs does.

   It should be possible to write an additional ioctl for behaviour
(B) which contains both min and max limits on each element of the key
3-tuple, *and* the current search state. That would reduce developer
confusion (given appropriate comments or documentation to explain what
the difference between the two is). However, I'm not sufficiently
convinced that it's actually necessary right now. I may change my tune
after I've started doing some of the more complex bits I'd thought of
doing with btrfs-gui, but for now, it's perfectly possible to use the
existing API without too much hassle.

   Hugo.

(*) The limited cases where both behaviours return the same set of
keys are:

(i_0, 0, 0) to (i_1, -1UL, -1UL)
(i, t_0, 0) to (i, t_1, -1UL)
(i, t, o_0) to (i, t, o_1)

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
  PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
    --- I get nervous when I see words like 'mayhaps' in a novel, ---    
                because I fear that just round the corner                
                          is lurking 'forsooth'                          

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: subvolumes missing from "btrfs subvolume list" output

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 657 bytes --]

On Wed, Jun 29, 2011 at 05:47:41PM +0100, Hugo Mills wrote:
> (*) The limited cases where both behaviours return the same set of
> keys are:
> 
> (i_0, 0, 0) to (i_1, -1UL, -1UL)
I clearly meant  (i_1,  255, -1UL) here...

> (i, t_0, 0) to (i, t_1, -1UL)
> (i, t, o_0) to (i, t, o_1)

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
  PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
    --- I get nervous when I see words like 'mayhaps' in a novel, ---    
                because I fear that just round the corner                
                          is lurking 'forsooth'                          

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: subvolumes missing from "btrfs subvolume list" output

[Goffredo Baroncelli]

Hi Hugo

On 06/29/2011 06:47 PM, Hugo Mills wrote:
> On Wed, Jun 29, 2011 at 12:16:06PM -0400, Josef Bacik wrote:
>> On 06/29/2011 11:00 AM, Stephane Chazelas wrote:
>>> 2011-06-29 15:37:47 +0100, Stephane Chazelas:
>>> [...]
>>>> I found
>>>> http://thread.gmane.org/gmane.comp.file-systems.btrfs/8123/focus=8208
>>>>
>>>> which looks like the same issue, with Li Zefan saying he had a
>>>> fix, but I couldn't find any mention that it was actually fixed.
>>>>
>>>> Has anybody got any update on that?
>>> [...]
>>>
>>> I've found
>>> http://thread.gmane.org/gmane.comp.file-systems.btrfs/8232
>>>
>>> but no corresponding fix or ioctl.c
>>> http://git.kernel.org/?p=linux/kernel/git/mason/btrfs-unstable.git;a=history;f=fs/btrfs/ioctl.c
>>>
>>> I'm under the impression that the issue has been forgotten
>>> about.
>>>
>>> From what I managed to gather though, it seems that what's on
>>> disk is correct, it's just the ioctl and/or "btrfs sub list"
>>> that's wrong. Am I right?
>>
>> Yeah, did you apply the patch from that thread and verify that it fixes
>> your problem?  Thanks,
> 
>    Note that changing this API will probably break btrfs-gui's listing
> of subvolumes...
> 
>    The issue with that patch is that there are two distinct behaviours
> that people want or expect with the tree-search ioctl:
> 
> (A) Return all items with keys which collate linearly between
>     (min_objectid, min_type, min_offset) and 
>     (max_objectid, max_type, max_offset)
> 
>     i.e. treating keys as indivisible objects and sorting lexically,
>     as the trees do.
> 
> (B) Return all items with keys (i, t, o) which fulfil the criteria
>     (min_objectid <= i <= max_objectid,
>      min_type <= t <= max_type,
>      min_offset <= o <= max_offset)
> 
>     i.e. treating keys as 3-tuples, and selecting from a rectilinear
>     subsset of the tuple space, which is natural for some
>     applications.
> 
>    Clearly, we can't do both with the same call (except for some
> limited cases (*)). However, different users expect different
> behaviours. The current behaviour is (A), which is the "natural"
> behaviour for tree searches within the btrfs code, and is (IMO) the
> right thing to be doing for an API like this.

looking at the function copy_to_sk() it seems that the key advance is
made on the following logic:

        if (key->offset < (u64)-1 && key->offset < sk->max_offset)
                key->offset++;
        else if (key->type < (u8)-1 && key->type < sk->max_type) {
                key->offset = 0;
                key->type++;
        } else if (key->objectid < (u64)-1 &&
	   key->objectid < sk->max_objectid) {
                key->offset = 0;
                key->type = 0;
                key->objectid++;

which to me it seems a bit different from the case A. In fact (if I read
the code correctly) *both* the following condition are always true

     (min_objectid, min_type, min_offset) <= key   and
     key < (max_objectid, max_type, max_offset)    and
     (key_objectid <= max_objectid                 and
      key_type <= max_type                         and
      key_offset <= max_offset)

In conclusion the code is an hybrid between A and B.





> 
>    It sounds to me like the user of the API needs to be fixed, not the
> ioctl itself -- possibly the author of the subvol scanning code
> assumed (B) when they were getting (A). Note that there is at least
> one other user of the ioctl outside btrfs-progs: btrfs-gui, which uses
> the ioctl for several things, one of which is enumerating subvolumes
> as btrfs-progs does.



> 
>    It should be possible to write an additional ioctl for behaviour
> (B) which contains both min and max limits on each element of the key
> 3-tuple, *and* the current search state. That would reduce developer
> confusion (given appropriate comments or documentation to explain what
> the difference between the two is). However, I'm not sufficiently
> convinced that it's actually necessary right now. I may change my tune
> after I've started doing some of the more complex bits I'd thought of
> doing with btrfs-gui, but for now, it's perfectly possible to use the
> existing API without too much hassle.
> 
>    Hugo.
> 
> (*) The limited cases where both behaviours return the same set of
> keys are:
> 
> (i_0, 0, 0) to (i_1, -1UL, -1UL)
> (i, t_0, 0) to (i, t_1, -1UL)
> (i, t, o_0) to (i, t, o_1)
> 

Re: subvolumes missing from "btrfs subvolume list" output

[Li Zefan]

Stephane Chazelas wrote:
> 2011-06-29 15:37:47 +0100, Stephane Chazelas:
> [...]
>> I found
>> http://thread.gmane.org/gmane.comp.file-systems.btrfs/8123/focus=8208
>>
>> which looks like the same issue, with Li Zefan saying he had a
>> fix, but I couldn't find any mention that it was actually fixed.
>>
>> Has anybody got any update on that?
> [...]
> 
> I've found
> http://thread.gmane.org/gmane.comp.file-systems.btrfs/8232
> 

After that, I posted a patch to fix btrfs-progs, which Chris aggreed
on:

http://marc.info/?l=linux-btrfs&m=129238454714319&w=2

> but no corresponding fix or ioctl.c
> http://git.kernel.org/?p=linux/kernel/git/mason/btrfs-unstable.git;a=history;f=fs/btrfs/ioctl.c
> 
> I'm under the impression that the issue has been forgotten
> about.
> 
>>From what I managed to gather though, it seems that what's on
> disk is correct, it's just the ioctl and/or "btrfs sub list"
> that's wrong. Am I right?
> 
> (btw, I forgot to mention the kernel version: 3.0rc4 amd64,
> btrfs tools from git)
> 

Re: subvolumes missing from "btrfs subvolume list" output

[Stephane Chazelas]

2011-06-30 08:47:38 +0800, Li Zefan:
> Stephane Chazelas wrote:
> > 2011-06-29 15:37:47 +0100, Stephane Chazelas:
> > [...]
> >> I found
> >> http://thread.gmane.org/gmane.comp.file-systems.btrfs/8123/focus=8208
> >>
> >> which looks like the same issue, with Li Zefan saying he had a
> >> fix, but I couldn't find any mention that it was actually fixed.
> >>
> >> Has anybody got any update on that?
> > [...]
> > 
> > I've found
> > http://thread.gmane.org/gmane.comp.file-systems.btrfs/8232
> > 
> 
> After that, I posted a patch to fix btrfs-progs, which Chris aggreed
> on:
> 
> http://marc.info/?l=linux-btrfs&m=129238454714319&w=2
[...]

Great. Thanks a lot

It fixes my problem indeed.

Which brings me to my next question: where to find the latest
btrfs-progs if not at
git://git.kernel.org/pub/scm/linux/kernel/git/mason/btrfs-progs-unstable.git
(that one hasn't been modified in 8 months)?

Have changes for read-only subvolumes, per-directory settings...
been applied to some publicly available repository?

Cheers,
Stephane

Re: subvolumes missing from "btrfs subvolume list" output

[Andreas Philipp]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
 
On 30.06.2011 10:40, Stephane Chazelas wrote:
> 2011-06-30 08:47:38 +0800, Li Zefan:
>> Stephane Chazelas wrote:
>>> 2011-06-29 15:37:47 +0100, Stephane Chazelas: [...]
>>>> I found
>>>> http://thread.gmane.org/gmane.comp.file-systems.btrfs/8123/focus=8208
>>>>
>>>>
>>>>
>>>>
which looks like the same issue, with Li Zefan saying he had a
>>>> fix, but I couldn't find any mention that it was actually
>>>> fixed.
>>>>
>>>> Has anybody got any update on that?
>>> [...]
>>>
>>> I've found
>>> http://thread.gmane.org/gmane.comp.file-systems.btrfs/8232
>>>
>>
>> After that, I posted a patch to fix btrfs-progs, which Chris
>> aggreed on:
>>
>> http://marc.info/?l=linux-btrfs&m=129238454714319&w=2
> [...]
>
> Great. Thanks a lot
>
> It fixes my problem indeed.
>
> Which brings me to my next question: where to find the latest
> btrfs-progs if not at
> git://git.kernel.org/pub/scm/linux/kernel/git/mason/btrfs-progs-unstable.git
>
>
>
(that one hasn't been modified in 8 months)?
>
> Have changes for read-only subvolumes, per-directory settings...
> been applied to some publicly available repository?
Hugo Mills keeps an integration branch with nearly all patches to
btrfs-progs applied.
See

http://www.spinics.net/lists/linux-btrfs/msg10594.html

and for the last update

http://www.spinics.net/lists/linux-btrfs/msg10890.html

Cheers,
Andreas
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
 
iQIcBAEBAgAGBQJODD9xAAoJEJIcBJ3+XkgiN1QP/RiO5KD4+wlEyVxe6WXq0xIE
GGsa4YrFWg/E9vKCw6LFVJAzdx3UdX3aM1EzDXoypX9jrtgEIbJbfjP635TAotDR
lYzvou0tovlxuyTBCbike7OVHz9faZwr91vpH9cNKFhDQMYXG0AIYO3gjv+lbTUc
m1CZNRaG1NUiyPTNoJi0Oj/e2cIAS7rhl1D26lLNfnwXMLA49a8EfaP526Qcg8eP
HqYWtBmm4cAPC7dNJbU9PELcJJ7hjnGE5ahZs/DwW6Usm8YEFZO6vA859oBp2REy
LwUCUcpnOSYjxD5iX4R7K7oPsHXdw8UkMACBzF1JgrqPS3rIi4bwRIp/ZA3M07AA
2B8eFD0oITxVK07vp9M64Sqms97V4cZxOtCkkCF+lEXppEaKj1lwpRoE4cmaAMW4
jA0sd8tgQw79Dy6/rBZyrxVJnXSmoWF3pisWIhHmFeAX+YnqT7ft7qXiWbQh7WMC
NFUw+1bagjshgZI+CxdY6stInJ+L+lSH4KTgtZb0mSxVnK440n/y/v5f9iDJ5js6
BkCZSYVjnite6wjZN5AYghZJfDCT4lXLN78dpeU6RWDD5/YlkJf4FkwFpHFVkHrO
M9x6cFRh74C5SiAPPWZUTT5g97mR4vuOzfTGwcSTv0ccaJzcxapUDZo50h30AbUA
TYTpSI2K2q3f+HvNJOPx
=ifgA
-----END PGP SIGNATURE-----

Re: subvolumes missing from "btrfs subvolume list" output

[Stephane Chazelas]

2011-06-30 11:18:42 +0200, Andreas Philipp:
[...]
> >> After that, I posted a patch to fix btrfs-progs, which Chris
> >> aggreed on:
> >>
> >> http://marc.info/?l=linux-btrfs&m=129238454714319&w=2
> > [...]
> >
> > Great. Thanks a lot
> >
> > It fixes my problem indeed.
> >
> > Which brings me to my next question: where to find the latest
> > btrfs-progs if not at
> > git://git.kernel.org/pub/scm/linux/kernel/git/mason/btrfs-progs-unstable.git
[...]
> Hugo Mills keeps an integration branch with nearly all patches to
> btrfs-progs applied.
> See
> 
> http://www.spinics.net/lists/linux-btrfs/msg10594.html
> 
> and for the last update
> 
> http://www.spinics.net/lists/linux-btrfs/msg10890.html
[...]

Thanks.

It might be worth adding a link to that to
https://btrfs.wiki.kernel.org/index.php/Btrfs_source_repositories

Note that it (integration-20110626) doesn't seem to include the fix in
http://marc.info/?l=linux-btrfs&m=129238454714319&w=2 though.

-- 
Stephane

Re: subvolumes missing from "btrfs subvolume list" output

[Andreas Philipp]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
 
On 30.06.2011 12:43, Stephane Chazelas wrote:
> 2011-06-30 11:18:42 +0200, Andreas Philipp: [...]
>>>> After that, I posted a patch to fix btrfs-progs, which Chris
>>>> aggreed on:
>>>>
>>>> http://marc.info/?l=linux-btrfs&m=129238454714319&w=2
>>> [...]
>>>
>>> Great. Thanks a lot
>>>
>>> It fixes my problem indeed.
>>>
>>> Which brings me to my next question: where to find the latest
>>> btrfs-progs if not at
>>>
git://git.kernel.org/pub/scm/linux/kernel/git/mason/btrfs-progs-unstable.git
>
>>>
[...]
>> Hugo Mills keeps an integration branch with nearly all patches
>> to btrfs-progs applied. See
>>
>> http://www.spinics.net/lists/linux-btrfs/msg10594.html
>>
>> and for the last update
>>
>> http://www.spinics.net/lists/linux-btrfs/msg10890.html
> [...]
>
> Thanks.
>
> It might be worth adding a link to that to
> https://btrfs.wiki.kernel.org/index.php/Btrfs_source_repositories
>
> Note that it (integration-20110626) doesn't seem to include the fix
> in http://marc.info/?l=linux-btrfs&m=129238454714319&w=2 though.
Hi Hugo,

Can you please include that fix in the next release of your
integration branch for btrfs-progs-unstable?

Thanks,
Andreas
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
 
iQIcBAEBAgAGBQJODFWKAAoJEJIcBJ3+Xkgi9VUQAI178VUW93+lHZV03YooS36z
YzCSd9UE4AK8n1H58gpQ2OWuLarTEkVB2iV2zn80DhKZioAlaJYA+mU9LG+c+dU6
IfwiyWrCFlaoyQn62gdmlFwgHcPglViSZX35LXuUbxdVzNM/KuCamp++ViL8TOZm
9vPd4UW98DxnvY+d4ui59Vt+5RWLxu5KHv+kRzXb5V4cMPRFmA4wrfyXkN8UdYLe
vJcJMFEy9JV2/fG+9thL+oglrb9pBkasHCXpoedUmJetf3eXpfnSZSiHRnYBlJY9
+dw1e4eh9uerJlwmCsqP1/TtGtyitwE8M/gXI8i+SkQKDNNqqa6RNf0axPNxfI9J
6jsqTzJCKcsV5VbdWMGJSEFanYmQ8rAI0BxJ+FrDlJIwZImnokFbxhrxUcYM4SpM
nNzbLV9bYHgx2LhotV+Iwoyfgb/S2vnnOnMoGs/LuaeDVO3fBnp1Ft1Ac6fi4T9q
wv/ot2TJ9tjLWGQOp1ACI+LzKd/DhtFs8p4NWaK/Tgc4Uy4VmcE+uLSIWh10s9Sg
UT0bEZMr7AXuDG9+QsjdnuOdLt/RjR2QAu4k3S3vU4ZHjGkYBXvYZd2pmaxCqJb1
aKtyGMf/apOYOWWyc8ia7dmm/dwA0plIjN30lWXfbx9YGlAzc3U4KYfgVyxfHjKx
lJkYHPcVpTSy4Y0AP6UJ
=uc4W
-----END PGP SIGNATURE-----

Re: subvolumes missing from "btrfs subvolume list" output

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 1601 bytes --]

On Thu, Jun 30, 2011 at 12:52:59PM +0200, Andreas Philipp wrote:
> 
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>  
> On 30.06.2011 12:43, Stephane Chazelas wrote:
> > 2011-06-30 11:18:42 +0200, Andreas Philipp: [...]
> >>>> After that, I posted a patch to fix btrfs-progs, which Chris
> >>>> aggreed on:
> >>>>
> >>>> http://marc.info/?l=linux-btrfs&m=129238454714319&w=2
> >>> [...]
> >>>
> >>> Great. Thanks a lot
> >>>
> >>> It fixes my problem indeed.
> >>>
> >>> Which brings me to my next question: where to find the latest
> >>> btrfs-progs if not at
> >>>
> git://git.kernel.org/pub/scm/linux/kernel/git/mason/btrfs-progs-unstable.git
> >
> >>>
> [...]
> >> Hugo Mills keeps an integration branch with nearly all patches
> >> to btrfs-progs applied. See
> >>
> >> http://www.spinics.net/lists/linux-btrfs/msg10594.html
> >>
> >> and for the last update
> >>
> >> http://www.spinics.net/lists/linux-btrfs/msg10890.html
> > [...]
> >
> > Thanks.
> >
> > It might be worth adding a link to that to
> > https://btrfs.wiki.kernel.org/index.php/Btrfs_source_repositories
> >
> > Note that it (integration-20110626) doesn't seem to include the fix
> > in http://marc.info/?l=linux-btrfs&m=129238454714319&w=2 though.
> Hi Hugo,
> 
> Can you please include that fix in the next release of your
> integration branch for btrfs-progs-unstable?

   Yes, will do.

   Hugo.

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
  PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
       --- Someone's been throwing dead sheep down my Fun Well ---       

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: subvolumes missing from "btrfs subvolume list" output

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 1513 bytes --]

On Thu, Jun 30, 2011 at 11:43:40AM +0100, Stephane Chazelas wrote:
> 2011-06-30 11:18:42 +0200, Andreas Philipp:
> [...]
> > >> After that, I posted a patch to fix btrfs-progs, which Chris
> > >> aggreed on:
> > >>
> > >> http://marc.info/?l=linux-btrfs&m=129238454714319&w=2
> > > [...]
> > >
> > > Great. Thanks a lot
> > >
> > > It fixes my problem indeed.
> > >
> > > Which brings me to my next question: where to find the latest
> > > btrfs-progs if not at
> > > git://git.kernel.org/pub/scm/linux/kernel/git/mason/btrfs-progs-unstable.git
> [...]
> > Hugo Mills keeps an integration branch with nearly all patches to
> > btrfs-progs applied.
> > See
> > 
> > http://www.spinics.net/lists/linux-btrfs/msg10594.html
> > 
> > and for the last update
> > 
> > http://www.spinics.net/lists/linux-btrfs/msg10890.html
> [...]
> 
> Thanks.
> 
> It might be worth adding a link to that to
> https://btrfs.wiki.kernel.org/index.php/Btrfs_source_repositories
> 
> Note that it (integration-20110626) doesn't seem to include the fix in
> http://marc.info/?l=linux-btrfs&m=129238454714319&w=2 though.

   No, I didn't see it when I did my trawl through the mailing list
archives, because it wasn't marked as [PATCH]. I'll pull it in for the
next round of the integration tree, though.

   Hugo.

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
  PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
       --- Someone's been throwing dead sheep down my Fun Well ---       

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[PATCH] [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"

[Stephane Chazelas]

Looks like this was missing in integration-20110626 for the
readonly snapshot patch:

diff --git a/btrfs.c b/btrfs.c
index e117172..be6ece5 100644
--- a/btrfs.c
+++ b/btrfs.c
@@ -49,7 +49,7 @@ static struct Command commands[] = {
 	/*
 		avoid short commands different for the case only
 	*/
-	{ do_clone, 2,
+	{ do_clone, -1,
 	  "subvolume snapshot", "[-r] <source> [<dest>/]<name>\n"
 		"Create a writable/readonly snapshot of the subvolume <source> with\n"
 		"the name <name> in the <dest> directory.",

Without that, "btrfs sub snap -r x y" would fail as it's not *2*
arguments.

-- 
Stephane

Re: [PATCH] [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]

On Thu, Jun 30, 2011 at 01:34:38PM +0100, Stephane Chazelas wrote:
> Looks like this was missing in integration-20110626 for the
> readonly snapshot patch:
> 
> diff --git a/btrfs.c b/btrfs.c
> index e117172..be6ece5 100644
> --- a/btrfs.c
> +++ b/btrfs.c
> @@ -49,7 +49,7 @@ static struct Command commands[] = {
>  	/*
>  		avoid short commands different for the case only
>  	*/
> -	{ do_clone, 2,
> +	{ do_clone, -1,
>  	  "subvolume snapshot", "[-r] <source> [<dest>/]<name>\n"
>  		"Create a writable/readonly snapshot of the subvolume <source> with\n"
>  		"the name <name> in the <dest> directory.",
> 
> Without that, "btrfs sub snap -r x y" would fail as it's not *2*
> arguments.

   Thanks. Added to the queue.

   Hugo.

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
  PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
   --- "How deep will this sub go?" "Oh,  she'll go all the way to ---   
                    the bottom if we don't stop her."                    

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"

[Andreas Philipp]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
 
On 30.06.2011 14:34, Stephane Chazelas wrote:
> Looks like this was missing in integration-20110626 for the
> readonly snapshot patch:
>
> diff --git a/btrfs.c b/btrfs.c
> index e117172..be6ece5 100644
> --- a/btrfs.c
> +++ b/btrfs.c
> @@ -49,7 +49,7 @@ static struct Command commands[] = {
> /*
> avoid short commands different for the case only
> */
> - { do_clone, 2,
> + { do_clone, -1,
> "subvolume snapshot", "[-r] <source> [<dest>/]<name>\n"
> "Create a writable/readonly snapshot of the subvolume <source> with\n"
> "the name <name> in the <dest> directory.",
>
> Without that, "btrfs sub snap -r x y" would fail as it's not *2*
> arguments.
Unfortunately, this is not correct either. "-1" means that the minimum
number of arguments is 1 and since we need at least <source> and
<name> this is 2. So the correct version should be -2.

Thanks,
Andreas
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
 
iQIcBAEBAgAGBQJODOKyAAoJEJIcBJ3+XkgiEzEP/1vXuvIqPvWQ6Yg3KGW31vZB
80BZRUIKJQel2VqZ8NN4mNJoXPsQ3r21gTv9aZQwHHnCRszBCDqo5uhJUIKfVsWX
WIOkQo6uJLUeO5tCwuIGBDwzy8WBgFyCNhUT06GVNJxNP2WZddsp3Wo540Mmmoof
en3+4SPC5godW2+t80811le+bfQpZ1N+Q2MXLTx0ilvt3y2cCjoV26yIhRj1mzOn
QBKRZCwvMafmE9jBdZLIx4h+W3agT3EWQj9F7c4ZsJFsrc5SJWtjMNZrWg8aRtYR
YFcHwuAH4IzVHxB0a84tiCtH+N+fnmQpXUWUZGjJLODgmJnJ+wq/vi1ek2J8Dw+c
pF3E+U6oy/bX/sQ7Ef/Zxs3qlp88hgLypd9Y2/Lk7i0XmKNYxpg9KisMiAS+Ensa
ucb0lNxB1rCSEtN48JfWJ55aA0Yh4B1WompXuRseekcVKFuOW/OqBkE73Q0YBSQo
FogqVGvrdmfgAeC8Uft83oo1zXXNYoxIpP5PjPgmpgwBQihZDHfVqbagFhKnRsRq
bsXyzl2C7SkgVUxtuD3/Co3uyFm/So9MsgJmkcDDGo4APhUGxSOxGqhWo49C/1CZ
xfUSC1+rYnzTwK+5vhjbJjjXTdjaRGprcEH0803+cVikPdOSYfKsztrf+9OGSAp7
J+vEuVEESjhEcn2ySfSx
=bSbf
-----END PGP SIGNATURE-----

Re: [PATCH] [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 1431 bytes --]

On Thu, Jun 30, 2011 at 10:55:15PM +0200, Andreas Philipp wrote:
> On 30.06.2011 14:34, Stephane Chazelas wrote:
> > Looks like this was missing in integration-20110626 for the
> > readonly snapshot patch:
> >
> > diff --git a/btrfs.c b/btrfs.c
> > index e117172..be6ece5 100644
> > --- a/btrfs.c
> > +++ b/btrfs.c
> > @@ -49,7 +49,7 @@ static struct Command commands[] = {
> > /*
> > avoid short commands different for the case only
> > */
> > - { do_clone, 2,
> > + { do_clone, -1,
> > "subvolume snapshot", "[-r] <source> [<dest>/]<name>\n"
> > "Create a writable/readonly snapshot of the subvolume <source> with\n"
> > "the name <name> in the <dest> directory.",
> >
> > Without that, "btrfs sub snap -r x y" would fail as it's not *2*
> > arguments.
> Unfortunately, this is not correct either. "-1" means that the minimum
> number of arguments is 1 and since we need at least <source> and
> <name> this is 2. So the correct version should be -2.

   OK, I'll fix that here, as the patch is part of my pull request for
Chris. (I saw the [] around <dest> but missed that <name> was
mandatory... it's been a long day).

   Hugo.

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
  PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
   --- Nothing wrong with being written in Perl... Some of my best ---   
                      friends are written in Perl.                       

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: subvolumes missing from "btrfs subvolume list" output

[Li Zefan]

>>> Hugo Mills keeps an integration branch with nearly all patches to
>>> btrfs-progs applied.
>>> See
>>>
>>> http://www.spinics.net/lists/linux-btrfs/msg10594.html
>>>
>>> and for the last update
>>>
>>> http://www.spinics.net/lists/linux-btrfs/msg10890.html
>> [...]
>>
>> Thanks.
>>
>> It might be worth adding a link to that to
>> https://btrfs.wiki.kernel.org/index.php/Btrfs_source_repositories
>>
>> Note that it (integration-20110626) doesn't seem to include the fix in
>> http://marc.info/?l=linux-btrfs&m=129238454714319&w=2 though.
> 
>    No, I didn't see it when I did my trawl through the mailing list
> archives, because it wasn't marked as [PATCH]. I'll pull it in for the
> next round of the integration tree, though.
> 

This is my SOB for the patch:

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>

[PATCH] Re: [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"

[Stephane Chazelas]

2011-06-30 22:55:15 +0200, Andreas Philipp:
> 
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>  
> On 30.06.2011 14:34, Stephane Chazelas wrote:
> > Looks like this was missing in integration-20110626 for the
> > readonly snapshot patch:
> >
> > diff --git a/btrfs.c b/btrfs.c
> > index e117172..be6ece5 100644
> > --- a/btrfs.c
> > +++ b/btrfs.c
> > @@ -49,7 +49,7 @@ static struct Command commands[] = {
> > /*
> > avoid short commands different for the case only
> > */
> > - { do_clone, 2,
> > + { do_clone, -1,
> > "subvolume snapshot", "[-r] <source> [<dest>/]<name>\n"
> > "Create a writable/readonly snapshot of the subvolume <source> with\n"
> > "the name <name> in the <dest> directory.",
> >
> > Without that, "btrfs sub snap -r x y" would fail as it's not *2*
> > arguments.
> Unfortunately, this is not correct either. "-1" means that the minimum
> number of arguments is 1 and since we need at least <source> and
> <name> this is 2. So the correct version should be -2.
[...]

Sorry, without looking closely at the source, I assumed -1 meant
defer the checking to the subcommand handler.

do_clone will indeed return an error if the number of arguments
is less than expected (so with -2, you'll get a different error
message if you do "btrfs sub snap -r foo" or "btrfs sub snap
foo") , but will not if it's more than expected by the way.

So the patch should probably be:

diff --git a/btrfs.c b/btrfs.c
index e117172..b50c58a 100644
--- a/btrfs.c
+++ b/btrfs.c
@@ -49,7 +49,7 @@ static struct Command commands[] = {
 	/*
 		avoid short commands different for the case only
 	*/
-	{ do_clone, 2,
+	{ do_clone, -2,
 	  "subvolume snapshot", "[-r] <source> [<dest>/]<name>\n"
 		"Create a writable/readonly snapshot of the subvolume <source> with\n"
 		"the name <name> in the <dest> directory.",
diff --git a/btrfs_cmds.c b/btrfs_cmds.c
index 1d18c59..3415afc 100644
--- a/btrfs_cmds.c
+++ b/btrfs_cmds.c
@@ -355,7 +355,7 @@ int do_clone(int argc, char **argv)
 			return 1;
 		}
 	}
-	if (argc - optind < 2) {
+	if (argc - optind != 2) {
 		fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
 		free(argv);
 		return 1;

Cheers,
Stephane

Re: [PATCH] Re: [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"

[Andreas Philipp]

On 01.07.2011 10:26, Stephane Chazelas wrote:
> 2011-06-30 22:55:15 +0200, Andreas Philipp:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>  
>> On 30.06.2011 14:34, Stephane Chazelas wrote:
>>> Looks like this was missing in integration-20110626 for the
>>> readonly snapshot patch:
>>>
>>> diff --git a/btrfs.c b/btrfs.c
>>> index e117172..be6ece5 100644
>>> --- a/btrfs.c
>>> +++ b/btrfs.c
>>> @@ -49,7 +49,7 @@ static struct Command commands[] = {
>>> /*
>>> avoid short commands different for the case only
>>> */
>>> - { do_clone, 2,
>>> + { do_clone, -1,
>>> "subvolume snapshot", "[-r] <source> [<dest>/]<name>\n"
>>> "Create a writable/readonly snapshot of the subvolume <source> with\n"
>>> "the name <name> in the <dest> directory.",
>>>
>>> Without that, "btrfs sub snap -r x y" would fail as it's not *2*
>>> arguments.
>> Unfortunately, this is not correct either. "-1" means that the minimum
>> number of arguments is 1 and since we need at least <source> and
>> <name> this is 2. So the correct version should be -2.
> [...]
>
> Sorry, without looking closely at the source, I assumed -1 meant
> defer the checking to the subcommand handler.
>
> do_clone will indeed return an error if the number of arguments
> is less than expected (so with -2, you'll get a different error
> message if you do "btrfs sub snap -r foo" or "btrfs sub snap
> foo") , but will not if it's more than expected by the way.
>
> So the patch should probably be:
>
> diff --git a/btrfs.c b/btrfs.c
> index e117172..b50c58a 100644
> --- a/btrfs.c
> +++ b/btrfs.c
> @@ -49,7 +49,7 @@ static struct Command commands[] = {
>  	/*
>  		avoid short commands different for the case only
>  	*/
> -	{ do_clone, 2,
> +	{ do_clone, -2,
>  	  "subvolume snapshot", "[-r] <source> [<dest>/]<name>\n"
>  		"Create a writable/readonly snapshot of the subvolume <source> with\n"
>  		"the name <name> in the <dest> directory.",
> diff --git a/btrfs_cmds.c b/btrfs_cmds.c
> index 1d18c59..3415afc 100644
> --- a/btrfs_cmds.c
> +++ b/btrfs_cmds.c
> @@ -355,7 +355,7 @@ int do_clone(int argc, char **argv)
>  			return 1;
>  		}
>  	}
> -	if (argc - optind < 2) {
> +	if (argc - optind != 2) {
>  		fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
>  		free(argv);
>  		return 1;
>
Thanks for having another look at this. You are perfectly right. Should
we patch my patch or should I rework a corrected version? What do you
think Hugo?

Cheers,
Andreas

Re: [PATCH] Re: [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 3205 bytes --]

On Fri, Jul 01, 2011 at 12:13:30PM +0200, Andreas Philipp wrote:
> On 01.07.2011 10:26, Stephane Chazelas wrote:
> > 2011-06-30 22:55:15 +0200, Andreas Philipp:
> >> -----BEGIN PGP SIGNED MESSAGE-----
> >> Hash: SHA1
> >>  
> >> On 30.06.2011 14:34, Stephane Chazelas wrote:
> >>> Looks like this was missing in integration-20110626 for the
> >>> readonly snapshot patch:
> >>>
> >>> diff --git a/btrfs.c b/btrfs.c
> >>> index e117172..be6ece5 100644
> >>> --- a/btrfs.c
> >>> +++ b/btrfs.c
> >>> @@ -49,7 +49,7 @@ static struct Command commands[] = {
> >>> /*
> >>> avoid short commands different for the case only
> >>> */
> >>> - { do_clone, 2,
> >>> + { do_clone, -1,
> >>> "subvolume snapshot", "[-r] <source> [<dest>/]<name>\n"
> >>> "Create a writable/readonly snapshot of the subvolume <source> with\n"
> >>> "the name <name> in the <dest> directory.",
> >>>
> >>> Without that, "btrfs sub snap -r x y" would fail as it's not *2*
> >>> arguments.
> >> Unfortunately, this is not correct either. "-1" means that the minimum
> >> number of arguments is 1 and since we need at least <source> and
> >> <name> this is 2. So the correct version should be -2.
> > [...]
> >
> > Sorry, without looking closely at the source, I assumed -1 meant
> > defer the checking to the subcommand handler.
> >
> > do_clone will indeed return an error if the number of arguments
> > is less than expected (so with -2, you'll get a different error
> > message if you do "btrfs sub snap -r foo" or "btrfs sub snap
> > foo") , but will not if it's more than expected by the way.
> >
> > So the patch should probably be:
> >
> > diff --git a/btrfs.c b/btrfs.c
> > index e117172..b50c58a 100644
> > --- a/btrfs.c
> > +++ b/btrfs.c
> > @@ -49,7 +49,7 @@ static struct Command commands[] = {
> >  	/*
> >  		avoid short commands different for the case only
> >  	*/
> > -	{ do_clone, 2,
> > +	{ do_clone, -2,
> >  	  "subvolume snapshot", "[-r] <source> [<dest>/]<name>\n"
> >  		"Create a writable/readonly snapshot of the subvolume <source> with\n"
> >  		"the name <name> in the <dest> directory.",
> > diff --git a/btrfs_cmds.c b/btrfs_cmds.c
> > index 1d18c59..3415afc 100644
> > --- a/btrfs_cmds.c
> > +++ b/btrfs_cmds.c
> > @@ -355,7 +355,7 @@ int do_clone(int argc, char **argv)
> >  			return 1;
> >  		}
> >  	}
> > -	if (argc - optind < 2) {
> > +	if (argc - optind != 2) {
> >  		fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
> >  		free(argv);
> >  		return 1;
> >
> Thanks for having another look at this. You are perfectly right. Should
> we patch my patch or should I rework a corrected version? What do you
> think Hugo?

   Could you send a follow-up patch with just the second hunk, please?
I screwed up the process with this (processing patches too quickly to
catch the review), and I've already published the patch with the first
hunk, above, into the for-chris branch.

   Hugo.

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
  PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
    --- It's not so much an afterlife, more a sort of après vie. ---    

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] Re: [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"

[Stephane Chazelas]

2011-07-01 11:42:23 +0100, Hugo Mills:
[...]
> > > diff --git a/btrfs.c b/btrfs.c
> > > index e117172..b50c58a 100644
> > > --- a/btrfs.c
> > > +++ b/btrfs.c
> > > @@ -49,7 +49,7 @@ static struct Command commands[] = {
> > >  	/*
> > >  		avoid short commands different for the case only
> > >  	*/
> > > -	{ do_clone, 2,
> > > +	{ do_clone, -2,
> > >  	  "subvolume snapshot", "[-r] <source> [<dest>/]<name>\n"
> > >  		"Create a writable/readonly snapshot of the subvolume <source> with\n"
> > >  		"the name <name> in the <dest> directory.",
> > > diff --git a/btrfs_cmds.c b/btrfs_cmds.c
> > > index 1d18c59..3415afc 100644
> > > --- a/btrfs_cmds.c
> > > +++ b/btrfs_cmds.c
> > > @@ -355,7 +355,7 @@ int do_clone(int argc, char **argv)
> > >  			return 1;
> > >  		}
> > >  	}
> > > -	if (argc - optind < 2) {
> > > +	if (argc - optind != 2) {
> > >  		fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
> > >  		free(argv);
> > >  		return 1;
> > >
> > Thanks for having another look at this. You are perfectly right. Should
> > we patch my patch or should I rework a corrected version? What do you
> > think Hugo?
> 
>    Could you send a follow-up patch with just the second hunk, please?
> I screwed up the process with this (processing patches too quickly to
> catch the review), and I've already published the patch with the first
> hunk, above, into the for-chris branch.

Hugo, not sure what you mean nor whom you're talking to, but I
can certainly copy-paste the second hunk from above here:

diff --git a/btrfs_cmds.c b/btrfs_cmds.c
index 1d18c59..3415afc 100644
--- a/btrfs_cmds.c
+++ b/btrfs_cmds.c
@@ -355,7 +355,7 @@ int do_clone(int argc, char **argv)
 			return 1;
 		}
 	}
-	if (argc - optind < 2) {
+	if (argc - optind != 2) {
 		fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
 		free(argv);
 		return 1;

Cheers,
Stephane

Re: [PATCH] Re: [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"

[Tsutomu Itoh]

Hi, Hugo,

I built your for-chris branch, and ran 'btrfs sub snap' command.
Then, I encountered following error message.

  # btrfs sub snap Dir-1 Dir-2
  Invalid arguments for subvolume snapshot

commit:8b4c2a22bff85f86af44587973c8da8c29a67ffc is wrong, I think.

(2011/07/01 21:55), Stephane Chazelas wrote:
> 2011-07-01 11:42:23 +0100, Hugo Mills:
> [...]
>>>> diff --git a/btrfs.c b/btrfs.c
>>>> index e117172..b50c58a 100644
>>>> --- a/btrfs.c
>>>> +++ b/btrfs.c
>>>> @@ -49,7 +49,7 @@ static struct Command commands[] = {
>>>>  	/*
>>>>  		avoid short commands different for the case only
>>>>  	*/
>>>> -	{ do_clone, 2,
>>>> +	{ do_clone, -2,
>>>>  	  "subvolume snapshot", "[-r] <source> [<dest>/]<name>\n"
>>>>  		"Create a writable/readonly snapshot of the subvolume <source> with\n"
>>>>  		"the name <name> in the <dest> directory.",
>>>> diff --git a/btrfs_cmds.c b/btrfs_cmds.c
>>>> index 1d18c59..3415afc 100644
>>>> --- a/btrfs_cmds.c
>>>> +++ b/btrfs_cmds.c
>>>> @@ -355,7 +355,7 @@ int do_clone(int argc, char **argv)
>>>>  			return 1;
>>>>  		}
>>>>  	}
>>>> -	if (argc - optind < 2) {
>>>> +	if (argc - optind != 2) {
>>>>  		fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
>>>>  		free(argv);
>>>>  		return 1;
>>>>
>>> Thanks for having another look at this. You are perfectly right. Should
>>> we patch my patch or should I rework a corrected version? What do you
>>> think Hugo?
>>
>>    Could you send a follow-up patch with just the second hunk, please?
>> I screwed up the process with this (processing patches too quickly to
>> catch the review), and I've already published the patch with the first
>> hunk, above, into the for-chris branch.
> 
> Hugo, not sure what you mean nor whom you're talking to, but I
> can certainly copy-paste the second hunk from above here:
> 
> diff --git a/btrfs_cmds.c b/btrfs_cmds.c
> index 1d18c59..3415afc 100644
> --- a/btrfs_cmds.c
> +++ b/btrfs_cmds.c
> @@ -355,7 +355,7 @@ int do_clone(int argc, char **argv)
>  			return 1;
>  		}
>  	}
> -	if (argc - optind < 2) {

> +	if (argc - optind != 2) {

I think that '3' is correct.

Thanks,
Tsutomu

>  		fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
>  		free(argv);
>  		return 1;
> 
> Cheers,
> Stephane

Re: [PATCH] Re: [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"

[Andreas Philipp]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
 
Hi,
 
You are right. Some time ago I have already sent a patch for this. Hugo,
can you please integrate it?
Thanks,
Andreas Philipp
 
[PATCH] check number of args for btrfs sub snap correctly
 
Check whether there are the right number of arguments (exatly 2 without
the flag -r) in the subcommand handler for the btrfs subvolume snapshot
command.
 
Signed-off-by: Andreas Philipp <philipp.andreas@gmail.com>
- ---
 btrfs_cmds.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
 
diff --git a/btrfs_cmds.c b/btrfs_cmds.c
index da5bd91..42c82f8 100644
- --- a/btrfs_cmds.c
+++ b/btrfs_cmds.c
@@ -354,7 +354,7 @@ int do_clone(int argc, char **argv)
             return 1;
         }
     }
- -    if (argc - optind < 2) {
+    if (argc - optind != 3) {
         fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
         free(argv);
         return 1;
 
- --   
1.7.3.4
 
 
 
On 11.08.2011 06:30, Tsutomu Itoh wrote:     
> Hi, Hugo,
>
> I built your for-chris branch, and ran 'btrfs sub snap' command.
> Then, I encountered following error message.
>
> # btrfs sub snap Dir-1 Dir-2
> Invalid arguments for subvolume snapshot
>
> commit:8b4c2a22bff85f86af44587973c8da8c29a67ffc is wrong, I think.
>
> (2011/07/01 21:55), Stephane Chazelas wrote:
>> 2011-07-01 11:42:23 +0100, Hugo Mills:
>> [...]
>>>>> diff --git a/btrfs.c b/btrfs.c
>>>>> index e117172..b50c58a 100644
>>>>> --- a/btrfs.c
>>>>> +++ b/btrfs.c
>>>>> @@ -49,7 +49,7 @@ static struct Command commands[] = {
>>>>> /*
>>>>> avoid short commands different for the case only
>>>>> */
>>>>> - { do_clone, 2,
>>>>> + { do_clone, -2,
>>>>> "subvolume snapshot", "[-r] <source> [<dest>/]<name>\n"
>>>>> "Create a writable/readonly snapshot of the subvolume <source> with\n"
>>>>> "the name <name> in the <dest> directory.",
>>>>> diff --git a/btrfs_cmds.c b/btrfs_cmds.c
>>>>> index 1d18c59..3415afc 100644
>>>>> --- a/btrfs_cmds.c
>>>>> +++ b/btrfs_cmds.c
>>>>> @@ -355,7 +355,7 @@ int do_clone(int argc, char **argv)
>>>>> return 1;
>>>>> }
>>>>> }
>>>>> - if (argc - optind < 2) {
>>>>> + if (argc - optind != 2) {
>>>>> fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
>>>>> free(argv);
>>>>> return 1;
>>>>>
>>>> Thanks for having another look at this. You are perfectly right. Should
>>>> we patch my patch or should I rework a corrected version? What do you
>>>> think Hugo?
>>> Could you send a follow-up patch with just the second hunk, please?
>>> I screwed up the process with this (processing patches too quickly to
>>> catch the review), and I've already published the patch with the first
>>> hunk, above, into the for-chris branch.
>> Hugo, not sure what you mean nor whom you're talking to, but I
>> can certainly copy-paste the second hunk from above here:
>>
>> diff --git a/btrfs_cmds.c b/btrfs_cmds.c
>> index 1d18c59..3415afc 100644
>> --- a/btrfs_cmds.c
>> +++ b/btrfs_cmds.c
>> @@ -355,7 +355,7 @@ int do_clone(int argc, char **argv)
>> return 1;
>> }
>> }
>> - if (argc - optind < 2) {
>> + if (argc - optind != 2) {
> I think that '3' is correct.
>
> Thanks,
> Tsutomu
>
>> fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
>> free(argv);
>> return 1;
>>
>> Cheers,
>> Stephane
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
 
iQIcBAEBAgAGBQJOQ3qTAAoJEJIcBJ3+XkgiYTsP/jS8jZoQQoVzpLClfQ4+xIbh
T1DLiEsu92fy+vxRSEUiFpSUcNlv6qfczwlOwmzWpWk/Cs01RP/XzBPAQc8qqZDy
JcEHq1Iteix9r9f7kMw2kA2UuRXrtuEiKjAYAOzp8OFoWHLB+S8yV1ajsRa9THOX
wZ+8wUkNuBfNFjJmwtirNpU0XJwm2QHVAI9n+7jj9asNVedY5tt6vw0YtFVpy2zS
FxUmGVoo+9+BAEiYkpZ9uvtXUNLojTwyFjMwbH4QAzJWrLTrMhXlj+SRS52mKt9Z
+Sr71QcIdhy+bb4oBDCjwB1SV9qyAoHTUs1yETzNRPAUgdnX41gZ6q/FYBDplmwZ
wLNiIa5MjO63SMN32CzLPQn/WbCzWIuGFkoYaBPYQu1ig7xZ3GulCmzhOqWMRfLn
FcN9GKF/LDcjaey9eTtgrZSp+Q+lRgsHbgg/2aMjZiGMYjOq/+EcFWXA9JfHQqSo
anQJU9WyUd4eO0ZYFMdRYeH81/+U6UktQVViQF4G9KbV3lbu3WQHhbbdFe2i7FXH
hWhaUdSEd6JmdKQWgqlV5XWo0hgMLO04oP8S5V03zEH6n5J4YU1wHDpZvsZTQbe8
3+k0far4o48GbcjUrMeRBkEuJiMOjNIHBXhjx5IAWkFGAQXvU29VA0IJ/XLFKjLA
0RhnC+XwTuaMSTYDzdxP
=oQbK
-----END PGP SIGNATURE-----

Re: [PATCH] Re: [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"

[Hugo Mills]

On Thu, Aug 11, 2011 at 08:45:40AM +0200, Andreas Philipp wrote:
> 
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>  
> Hi,
>  
> You are right. Some time ago I have already sent a patch for this. Hugo,
> can you please integrate it?

   I'm at a cricket match right now (India 224, England 122/0), so I
can't do much today, but I should get time tomorrow.

   Hugo.

> Thanks,
> Andreas Philipp
>  
> [PATCH] check number of args for btrfs sub snap correctly
>  
> Check whether there are the right number of arguments (exatly 2 without
> the flag -r) in the subcommand handler for the btrfs subvolume snapshot
> command.
>  
> Signed-off-by: Andreas Philipp <philipp.andreas@gmail.com>
> - ---
>  btrfs_cmds.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>  
> diff --git a/btrfs_cmds.c b/btrfs_cmds.c
> index da5bd91..42c82f8 100644
> - --- a/btrfs_cmds.c
> +++ b/btrfs_cmds.c
> @@ -354,7 +354,7 @@ int do_clone(int argc, char **argv)
>              return 1;
>          }
>      }
> - -    if (argc - optind < 2) {
> +    if (argc - optind != 3) {
>          fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
>          free(argv);
>          return 1;
>  
> - --   
> 1.7.3.4
>  
>  
>  
> On 11.08.2011 06:30, Tsutomu Itoh wrote:     
> > Hi, Hugo,
> >
> > I built your for-chris branch, and ran 'btrfs sub snap' command.
> > Then, I encountered following error message.
> >
> > # btrfs sub snap Dir-1 Dir-2
> > Invalid arguments for subvolume snapshot
> >
> > commit:8b4c2a22bff85f86af44587973c8da8c29a67ffc is wrong, I think.
> >
> > (2011/07/01 21:55), Stephane Chazelas wrote:
> >> 2011-07-01 11:42:23 +0100, Hugo Mills:
> >> [...]
> >>>>> diff --git a/btrfs.c b/btrfs.c
> >>>>> index e117172..b50c58a 100644
> >>>>> --- a/btrfs.c
> >>>>> +++ b/btrfs.c
> >>>>> @@ -49,7 +49,7 @@ static struct Command commands[] = {
> >>>>> /*
> >>>>> avoid short commands different for the case only
> >>>>> */
> >>>>> - { do_clone, 2,
> >>>>> + { do_clone, -2,
> >>>>> "subvolume snapshot", "[-r] <source> [<dest>/]<name>\n"
> >>>>> "Create a writable/readonly snapshot of the subvolume <source> with\n"
> >>>>> "the name <name> in the <dest> directory.",
> >>>>> diff --git a/btrfs_cmds.c b/btrfs_cmds.c
> >>>>> index 1d18c59..3415afc 100644
> >>>>> --- a/btrfs_cmds.c
> >>>>> +++ b/btrfs_cmds.c
> >>>>> @@ -355,7 +355,7 @@ int do_clone(int argc, char **argv)
> >>>>> return 1;
> >>>>> }
> >>>>> }
> >>>>> - if (argc - optind < 2) {
> >>>>> + if (argc - optind != 2) {
> >>>>> fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
> >>>>> free(argv);
> >>>>> return 1;
> >>>>>
> >>>> Thanks for having another look at this. You are perfectly right. Should
> >>>> we patch my patch or should I rework a corrected version? What do you
> >>>> think Hugo?
> >>> Could you send a follow-up patch with just the second hunk, please?
> >>> I screwed up the process with this (processing patches too quickly to
> >>> catch the review), and I've already published the patch with the first
> >>> hunk, above, into the for-chris branch.
> >> Hugo, not sure what you mean nor whom you're talking to, but I
> >> can certainly copy-paste the second hunk from above here:
> >>
> >> diff --git a/btrfs_cmds.c b/btrfs_cmds.c
> >> index 1d18c59..3415afc 100644
> >> --- a/btrfs_cmds.c
> >> +++ b/btrfs_cmds.c
> >> @@ -355,7 +355,7 @@ int do_clone(int argc, char **argv)
> >> return 1;
> >> }
> >> }
> >> - if (argc - optind < 2) {
> >> + if (argc - optind != 2) {
> > I think that '3' is correct.
> >
> > Thanks,
> > Tsutomu
> >
> >> fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
> >> free(argv);
> >> return 1;
> >>
> >> Cheers,
> >> Stephane
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (MingW32)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
>  
> iQIcBAEBAgAGBQJOQ3qTAAoJEJIcBJ3+XkgiYTsP/jS8jZoQQoVzpLClfQ4+xIbh
> T1DLiEsu92fy+vxRSEUiFpSUcNlv6qfczwlOwmzWpWk/Cs01RP/XzBPAQc8qqZDy
> JcEHq1Iteix9r9f7kMw2kA2UuRXrtuEiKjAYAOzp8OFoWHLB+S8yV1ajsRa9THOX
> wZ+8wUkNuBfNFjJmwtirNpU0XJwm2QHVAI9n+7jj9asNVedY5tt6vw0YtFVpy2zS
> FxUmGVoo+9+BAEiYkpZ9uvtXUNLojTwyFjMwbH4QAzJWrLTrMhXlj+SRS52mKt9Z
> +Sr71QcIdhy+bb4oBDCjwB1SV9qyAoHTUs1yETzNRPAUgdnX41gZ6q/FYBDplmwZ
> wLNiIa5MjO63SMN32CzLPQn/WbCzWIuGFkoYaBPYQu1ig7xZ3GulCmzhOqWMRfLn
> FcN9GKF/LDcjaey9eTtgrZSp+Q+lRgsHbgg/2aMjZiGMYjOq/+EcFWXA9JfHQqSo
> anQJU9WyUd4eO0ZYFMdRYeH81/+U6UktQVViQF4G9KbV3lbu3WQHhbbdFe2i7FXH
> hWhaUdSEd6JmdKQWgqlV5XWo0hgMLO04oP8S5V03zEH6n5J4YU1wHDpZvsZTQbe8
> 3+k0far4o48GbcjUrMeRBkEuJiMOjNIHBXhjx5IAWkFGAQXvU29VA0IJ/XLFKjLA
> 0RhnC+XwTuaMSTYDzdxP
> =oQbK
> -----END PGP SIGNATURE-----
> 

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
  PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
      --- Normaliser unix c'est comme pasteuriser le Camembert ---