Stephane Chazelas
2011-Jun-29 14:37 UTC
subvolumes missing from "btrfs subvolume list" output
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 -- 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
Stephane Chazelas
2011-Jun-29 15:00 UTC
Re: subvolumes missing from "btrfs subvolume list" output
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 -- 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
Josef Bacik
2011-Jun-29 16:16 UTC
Re: subvolumes missing from "btrfs subvolume list" output
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 -- 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
Hugo Mills
2011-Jun-29 16:47 UTC
Re: subvolumes missing from "btrfs subvolume list" output
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''
Hugo Mills
2011-Jun-29 16:50 UTC
Re: subvolumes missing from "btrfs subvolume list" output
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''
Goffredo Baroncelli
2011-Jun-29 20:41 UTC
Re: subvolumes missing from "btrfs subvolume list" output
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) >-- 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
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) >-- 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
Stephane Chazelas
2011-Jun-30 08:40 UTC
Re: subvolumes missing from "btrfs subvolume list" output
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 -- 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
Andreas Philipp
2011-Jun-30 09:18 UTC
Re: subvolumes missing from "btrfs subvolume list" output
-----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----- -- 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
Stephane Chazelas
2011-Jun-30 10:43 UTC
Re: subvolumes missing from "btrfs subvolume list" output
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 -- 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
Andreas Philipp
2011-Jun-30 10:52 UTC
Re: subvolumes missing from "btrfs subvolume list" output
-----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----- -- 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
Hugo Mills
2011-Jun-30 10:54 UTC
Re: subvolumes missing from "btrfs subvolume list" output
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 ---
Hugo Mills
2011-Jun-30 10:58 UTC
Re: subvolumes missing from "btrfs subvolume list" output
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 ---
Stephane Chazelas
2011-Jun-30 12:34 UTC
[PATCH] [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"
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 -- 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
Hugo Mills
2011-Jun-30 13:07 UTC
Re: [PATCH] [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"
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."
Andreas Philipp
2011-Jun-30 20:55 UTC
Re: [PATCH] [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"
-----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----- -- 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
Hugo Mills
2011-Jun-30 21:09 UTC
Re: [PATCH] [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"
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.
>>> 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> -- 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
Stephane Chazelas
2011-Jul-01 08:26 UTC
[PATCH] Re: [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"
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 -- 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
Andreas Philipp
2011-Jul-01 10:13 UTC
Re: [PATCH] Re: [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"
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 -- 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
Hugo Mills
2011-Jul-01 10:42 UTC
Re: [PATCH] Re: [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"
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. ---
Stephane Chazelas
2011-Jul-01 12:55 UTC
Re: [PATCH] Re: [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"
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 -- 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
Tsutomu Itoh
2011-Aug-11 04:30 UTC
Re: [PATCH] Re: [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"
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-- 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
Andreas Philipp
2011-Aug-11 06:45 UTC
Re: [PATCH] Re: [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"
-----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----- -- 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
Hugo Mills
2011-Aug-11 12:40 UTC
Re: [PATCH] Re: [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"
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 --- -- 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