[PATCH 0/6] btrfs support part1: subvolume commands
Hi, This is the part1 of improving btrfs support. This series adds missing parameters to btrfs_subvolume_snapshot and btrfs_subvolume_create, and adds two new API btrfs_subvolume_get_default and btrfs_subvolume_show. Other parts will follow. Regards, Hu Hu Tao (6): btrfs: correct words about subvolume and snapshot btrfs: add optional parameter `ro' to btrfs_subvolume_snapshot btrfs: add optional parameter `qgroupid' to btrfs_subvolume_snapshot btrfs: add optional parameter `qgroupid' to btrfs_subvolume_create New API: btrfs_subvolume_get_default New API: btrfs_subvolume_show daemon/btrfs.c | 232 ++++++++++++++++++++++++++++++++++++++++++++++++++- generator/actions.ml | 44 +++++++--- src/MAX_PROC_NR | 2 +- 3 files changed, 263 insertions(+), 15 deletions(-) -- 1.9.3
btrfs_subvolume_create creates only subvolumes. btrfs_subvolume_delete deletes subvolumes or snapshots. Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- generator/actions.ml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/generator/actions.ml b/generator/actions.ml index 89332f5..fe492e6 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -10248,19 +10248,19 @@ of the snapshot, in the form C</path/to/dest/name>." }; ["btrfs_subvolume_create"; "/test1"]; ["btrfs_subvolume_delete"; "/test1"]]), [] ]; - shortdesc = "delete a btrfs snapshot"; + shortdesc = "delete a btrfs subvolume or snapshot"; longdesc = "\ -Delete the named btrfs subvolume." }; +Delete the named btrfs subvolume or snapshot." }; { defaults with name = "btrfs_subvolume_create"; style = RErr, [Pathname "dest"], []; proc_nr = Some 324; optional = Some "btrfs"; camel_name = "BTRFSSubvolumeCreate"; - shortdesc = "create a btrfs snapshot"; + shortdesc = "create a btrfs subvolume"; longdesc = "\ Create a btrfs subvolume. The C<dest> argument is the destination -directory and the name of the snapshot, in the form C</path/to/dest/name>." }; +directory and the name of the subvolume, in the form C</path/to/dest/name>." }; { defaults with name = "btrfs_subvolume_list"; -- 1.9.3
On Friday 21 November 2014 13:17:55 Hu Tao wrote:
btrfs_subvolume_create creates only subvolumes. btrfs_subvolume_delete deletes subvolumes or snapshots.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- generator/actions.ml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/generator/actions.ml b/generator/actions.ml index 89332f5..fe492e6 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -10248,19 +10248,19 @@ of the snapshot, in the form C</path/to/dest/name>." }; ["btrfs_subvolume_create"; "/test1"]; ["btrfs_subvolume_delete"; "/test1"]]), [] ]; - shortdesc = "delete a btrfs snapshot"; + shortdesc = "delete a btrfs subvolume or snapshot"; longdesc = "\ -Delete the named btrfs subvolume." }; +Delete the named btrfs subvolume or snapshot." };
{ defaults with name = "btrfs_subvolume_create"; style = RErr, [Pathname "dest"], []; proc_nr = Some 324; optional = Some "btrfs"; camel_name = "BTRFSSubvolumeCreate"; - shortdesc = "create a btrfs snapshot"; + shortdesc = "create a btrfs subvolume"; longdesc = "\ Create a btrfs subvolume. The C<dest> argument is the destination -directory and the name of the snapshot, in the form C</path/to/dest/name>." }; +directory and the name of the subvolume, in the form C</path/to/dest/name>." };
{ defaults with name = "btrfs_subvolume_list";
LGTM. -- Pino Toscano
On Fri, Nov 21, 2014 at 11:35:11AM +0100, Pino Toscano wrote:
On Friday 21 November 2014 13:17:55 Hu Tao wrote:
btrfs_subvolume_create creates only subvolumes. btrfs_subvolume_delete deletes subvolumes or snapshots.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- generator/actions.ml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/generator/actions.ml b/generator/actions.ml index 89332f5..fe492e6 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -10248,19 +10248,19 @@ of the snapshot, in the form C</path/to/dest/name>." }; ["btrfs_subvolume_create"; "/test1"]; ["btrfs_subvolume_delete"; "/test1"]]), [] ]; - shortdesc = "delete a btrfs snapshot"; + shortdesc = "delete a btrfs subvolume or snapshot"; longdesc = "\ -Delete the named btrfs subvolume." }; +Delete the named btrfs subvolume or snapshot." };
{ defaults with name = "btrfs_subvolume_create"; style = RErr, [Pathname "dest"], []; proc_nr = Some 324; optional = Some "btrfs"; camel_name = "BTRFSSubvolumeCreate"; - shortdesc = "create a btrfs snapshot"; + shortdesc = "create a btrfs subvolume"; longdesc = "\ Create a btrfs subvolume. The C<dest> argument is the destination -directory and the name of the snapshot, in the form C</path/to/dest/name>." }; +directory and the name of the subvolume, in the form C</path/to/dest/name>." };
{ defaults with name = "btrfs_subvolume_list";
LGTM.
This means I can have your Reviewed-by line? :) Regards, Hu
On Monday 24 November 2014 10:27:49 Hu Tao wrote:
On Fri, Nov 21, 2014 at 11:35:11AM +0100, Pino Toscano wrote:
On Friday 21 November 2014 13:17:55 Hu Tao wrote:
btrfs_subvolume_create creates only subvolumes. btrfs_subvolume_delete deletes subvolumes or snapshots.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> ---
generator/actions.ml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/generator/actions.ml b/generator/actions.ml index 89332f5..fe492e6 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -10248,19 +10248,19 @@ of the snapshot, in the form C</path/to/dest/name>." }; ["btrfs_subvolume_create"; "/test1"];
["btrfs_subvolume_delete"; "/test1"]]), []
];
- shortdesc = "delete a btrfs snapshot"; + shortdesc = "delete a btrfs subvolume or snapshot";
longdesc = "\
-Delete the named btrfs subvolume." }; +Delete the named btrfs subvolume or snapshot." };
{ defaults with
name = "btrfs_subvolume_create"; style = RErr, [Pathname "dest"], []; proc_nr = Some 324; optional = Some "btrfs"; camel_name = "BTRFSSubvolumeCreate";
- shortdesc = "create a btrfs snapshot"; + shortdesc = "create a btrfs subvolume";
longdesc = "\
Create a btrfs subvolume. The C<dest> argument is the destination
-directory and the name of the snapshot, in the form C</path/to/dest/name>." }; +directory and the name of the subvolume, in the form C</path/to/dest/name>." };
{ defaults with
name = "btrfs_subvolume_list";
LGTM.
This means I can have your Reviewed-by line? :)
Ah, forgot to say that it was pushed already on Friday. Thanks, -- Pino Toscano
Parameter `ro' is for creating readonly btrfs snapshot. Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- daemon/btrfs.c | 11 ++++++++++- generator/actions.ml | 4 ++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/daemon/btrfs.c b/daemon/btrfs.c index 7a4d43d..b630bce 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -208,7 +208,7 @@ do_mkfs_btrfs (char *const *devices, } int -do_btrfs_subvolume_snapshot (const char *source, const char *dest) +do_btrfs_subvolume_snapshot (const char *source, const char *dest, int ro) { const size_t MAX_ARGS = 64; const char *argv[MAX_ARGS]; @@ -231,6 +231,15 @@ do_btrfs_subvolume_snapshot (const char *source, const char *dest) ADD_ARG (argv, i, str_btrfs); ADD_ARG (argv, i, "subvolume"); ADD_ARG (argv, i, "snapshot"); + + /* Optional arguments. */ + if (!(optargs_bitmask & GUESTFS_BTRFS_SUBVOLUME_SNAPSHOT_RO_BITMASK)) + ro = 0; + + if (ro) { + ADD_ARG (argv, i, "-r"); + } + ADD_ARG (argv, i, source_buf); ADD_ARG (argv, i, dest_buf); ADD_ARG (argv, i, NULL); diff --git a/generator/actions.ml b/generator/actions.ml index fe492e6..850e58d 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -10217,7 +10217,7 @@ See C<guestfs_get_e2generation>." }; { defaults with name = "btrfs_subvolume_snapshot"; - style = RErr, [Pathname "source"; Pathname "dest"], []; + style = RErr, [Pathname "source"; Pathname "dest"], [OBool "ro"]; proc_nr = Some 322; optional = Some "btrfs"; camel_name = "BTRFSSubvolumeSnapshot"; tests = [ @@ -10228,7 +10228,7 @@ See C<guestfs_get_e2generation>." }; ["btrfs_subvolume_create"; "/test1"]; ["btrfs_subvolume_create"; "/test2"]; ["btrfs_subvolume_create"; "/dir/test3"]; - ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test4"]]), [] + ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test5"; "true"]]), [] ]; shortdesc = "create a writable btrfs snapshot"; longdesc = "\ -- 1.9.3
On Friday 21 November 2014 13:17:56 Hu Tao wrote:
Parameter `ro' is for creating readonly btrfs snapshot.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- daemon/btrfs.c | 11 ++++++++++- generator/actions.ml | 4 ++-- 2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c index 7a4d43d..b630bce 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -208,7 +208,7 @@ do_mkfs_btrfs (char *const *devices, }
int -do_btrfs_subvolume_snapshot (const char *source, const char *dest) +do_btrfs_subvolume_snapshot (const char *source, const char *dest, int ro) { const size_t MAX_ARGS = 64; const char *argv[MAX_ARGS]; @@ -231,6 +231,15 @@ do_btrfs_subvolume_snapshot (const char *source, const char *dest) ADD_ARG (argv, i, str_btrfs); ADD_ARG (argv, i, "subvolume"); ADD_ARG (argv, i, "snapshot"); + + /* Optional arguments. */ + if (!(optargs_bitmask & GUESTFS_BTRFS_SUBVOLUME_SNAPSHOT_RO_BITMASK)) + ro = 0; + + if (ro) { + ADD_ARG (argv, i, "-r"); + } +
I would make this bit simplier: if ((optargs_bitmask & GUESTFS_BTRFS_SUBVOLUME_SNAPSHOT_RO_BITMASK) && ro) ADD_ARG (argv, i, "-r"); Also, seen in the other patches as well, you don't need { ... } brackets for blocks of just one instructions.
diff --git a/generator/actions.ml b/generator/actions.ml index fe492e6..850e58d 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -10217,7 +10217,7 @@ See C<guestfs_get_e2generation>." };
{ defaults with name = "btrfs_subvolume_snapshot"; - style = RErr, [Pathname "source"; Pathname "dest"], []; + style = RErr, [Pathname "source"; Pathname "dest"], [OBool "ro"]; proc_nr = Some 322; optional = Some "btrfs"; camel_name = "BTRFSSubvolumeSnapshot"; tests = [
You must set once_had_no_optargs = true for this action, as it currently has no optional arguments; see generator/README. -- Pino Toscano
On Fri, Nov 21, 2014 at 11:38:54AM +0100, Pino Toscano wrote:
On Friday 21 November 2014 13:17:56 Hu Tao wrote:
Parameter `ro' is for creating readonly btrfs snapshot.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- daemon/btrfs.c | 11 ++++++++++- generator/actions.ml | 4 ++-- 2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c index 7a4d43d..b630bce 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -208,7 +208,7 @@ do_mkfs_btrfs (char *const *devices, }
int -do_btrfs_subvolume_snapshot (const char *source, const char *dest) +do_btrfs_subvolume_snapshot (const char *source, const char *dest, int ro) { const size_t MAX_ARGS = 64; const char *argv[MAX_ARGS]; @@ -231,6 +231,15 @@ do_btrfs_subvolume_snapshot (const char *source, const char *dest) ADD_ARG (argv, i, str_btrfs); ADD_ARG (argv, i, "subvolume"); ADD_ARG (argv, i, "snapshot"); + + /* Optional arguments. */ + if (!(optargs_bitmask & GUESTFS_BTRFS_SUBVOLUME_SNAPSHOT_RO_BITMASK)) + ro = 0; + + if (ro) { + ADD_ARG (argv, i, "-r"); + } +
I would make this bit simplier:
if ((optargs_bitmask & GUESTFS_BTRFS_SUBVOLUME_SNAPSHOT_RO_BITMASK) && ro) ADD_ARG (argv, i, "-r");
Also, seen in the other patches as well, you don't need { ... } brackets for blocks of just one instructions.
Okay.
diff --git a/generator/actions.ml b/generator/actions.ml index fe492e6..850e58d 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -10217,7 +10217,7 @@ See C<guestfs_get_e2generation>." };
{ defaults with name = "btrfs_subvolume_snapshot"; - style = RErr, [Pathname "source"; Pathname "dest"], []; + style = RErr, [Pathname "source"; Pathname "dest"], [OBool "ro"]; proc_nr = Some 322; optional = Some "btrfs"; camel_name = "BTRFSSubvolumeSnapshot"; tests = [
You must set once_had_no_optargs = true for this action, as it currently has no optional arguments; see generator/README.
Thanks for catching this! Regards, Hu
On Friday 21 November 2014 13:17:56 Hu Tao wrote:
diff --git a/generator/actions.ml b/generator/actions.ml index fe492e6..850e58d 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -10217,7 +10217,7 @@ See C<guestfs_get_e2generation>." };
{ defaults with name = "btrfs_subvolume_snapshot"; - style = RErr, [Pathname "source"; Pathname "dest"], []; + style = RErr, [Pathname "source"; Pathname "dest"], [OBool "ro"]; proc_nr = Some 322; optional = Some "btrfs"; camel_name = "BTRFSSubvolumeSnapshot"; tests = [ @@ -10228,7 +10228,7 @@ See C<guestfs_get_e2generation>." }; ["btrfs_subvolume_create"; "/test1"]; ["btrfs_subvolume_create"; "/test2"]; ["btrfs_subvolume_create"; "/dir/test3"]; - ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test4"]]), [] + ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test5"; "true"]]), [] ]; shortdesc = "create a writable btrfs snapshot"; longdesc = "\
Ah, forgot to add: can you please document the new "ro" optarg in the longdesc? -- Pino Toscano
On Fri, Nov 21, 2014 at 11:55:11AM +0100, Pino Toscano wrote:
On Friday 21 November 2014 13:17:56 Hu Tao wrote:
diff --git a/generator/actions.ml b/generator/actions.ml index fe492e6..850e58d 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -10217,7 +10217,7 @@ See C<guestfs_get_e2generation>." };
{ defaults with name = "btrfs_subvolume_snapshot"; - style = RErr, [Pathname "source"; Pathname "dest"], []; + style = RErr, [Pathname "source"; Pathname "dest"], [OBool "ro"]; proc_nr = Some 322; optional = Some "btrfs"; camel_name = "BTRFSSubvolumeSnapshot"; tests = [ @@ -10228,7 +10228,7 @@ See C<guestfs_get_e2generation>." }; ["btrfs_subvolume_create"; "/test1"]; ["btrfs_subvolume_create"; "/test2"]; ["btrfs_subvolume_create"; "/dir/test3"]; - ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test4"]]), [] + ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test5"; "true"]]), [] ]; shortdesc = "create a writable btrfs snapshot"; longdesc = "\
Ah, forgot to add: can you please document the new "ro" optarg in the longdesc?
Done. Regards, Hu
Parameter `qgroupid' is for adding the created snapshot to a qgroup. Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- daemon/btrfs.c | 8 +++++++- generator/actions.ml | 5 +++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/daemon/btrfs.c b/daemon/btrfs.c index b630bce..a20afca 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -208,7 +208,8 @@ do_mkfs_btrfs (char *const *devices, } int -do_btrfs_subvolume_snapshot (const char *source, const char *dest, int ro) +do_btrfs_subvolume_snapshot (const char *source, const char *dest, int ro, + const char *qgroupid) { const size_t MAX_ARGS = 64; const char *argv[MAX_ARGS]; @@ -240,6 +241,11 @@ do_btrfs_subvolume_snapshot (const char *source, const char *dest, int ro) ADD_ARG (argv, i, "-r"); } + if (optargs_bitmask & GUESTFS_BTRFS_SUBVOLUME_SNAPSHOT_QGROUPID_BITMASK) { + ADD_ARG (argv, i, "-i"); + ADD_ARG (argv, i, qgroupid); + } + ADD_ARG (argv, i, source_buf); ADD_ARG (argv, i, dest_buf); ADD_ARG (argv, i, NULL); diff --git a/generator/actions.ml b/generator/actions.ml index 850e58d..30b839c 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -10217,7 +10217,7 @@ See C<guestfs_get_e2generation>." }; { defaults with name = "btrfs_subvolume_snapshot"; - style = RErr, [Pathname "source"; Pathname "dest"], [OBool "ro"]; + style = RErr, [Pathname "source"; Pathname "dest"], [OBool "ro"; OString "qgroupid"]; proc_nr = Some 322; optional = Some "btrfs"; camel_name = "BTRFSSubvolumeSnapshot"; tests = [ @@ -10228,7 +10228,8 @@ See C<guestfs_get_e2generation>." }; ["btrfs_subvolume_create"; "/test1"]; ["btrfs_subvolume_create"; "/test2"]; ["btrfs_subvolume_create"; "/dir/test3"]; - ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test5"; "true"]]), [] + ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test5"; "true"; "NOARG"]; + ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test6"; ""; "0/1000"]]), [] ]; shortdesc = "create a writable btrfs snapshot"; longdesc = "\ -- 1.9.3
On Friday 21 November 2014 13:17:57 Hu Tao wrote:
Parameter `qgroupid' is for adding the created snapshot to a qgroup.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- daemon/btrfs.c | 8 +++++++- generator/actions.ml | 5 +++-- 2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c index b630bce..a20afca 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -208,7 +208,8 @@ do_mkfs_btrfs (char *const *devices, }
int -do_btrfs_subvolume_snapshot (const char *source, const char *dest, int ro) +do_btrfs_subvolume_snapshot (const char *source, const char *dest, int ro, + const char *qgroupid) { const size_t MAX_ARGS = 64; const char *argv[MAX_ARGS]; @@ -240,6 +241,11 @@ do_btrfs_subvolume_snapshot (const char *source, const char *dest, int ro) ADD_ARG (argv, i, "-r"); }
+ if (optargs_bitmask & GUESTFS_BTRFS_SUBVOLUME_SNAPSHOT_QGROUPID_BITMASK) { + ADD_ARG (argv, i, "-i"); + ADD_ARG (argv, i, qgroupid); + } + ADD_ARG (argv, i, source_buf); ADD_ARG (argv, i, dest_buf); ADD_ARG (argv, i, NULL); diff --git a/generator/actions.ml b/generator/actions.ml index 850e58d..30b839c 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -10217,7 +10217,7 @@ See C<guestfs_get_e2generation>." };
{ defaults with name = "btrfs_subvolume_snapshot"; - style = RErr, [Pathname "source"; Pathname "dest"], [OBool "ro"]; + style = RErr, [Pathname "source"; Pathname "dest"], [OBool "ro"; OString "qgroupid"]; proc_nr = Some 322; optional = Some "btrfs"; camel_name = "BTRFSSubvolumeSnapshot"; tests = [ @@ -10228,7 +10228,8 @@ See C<guestfs_get_e2generation>." }; ["btrfs_subvolume_create"; "/test1"]; ["btrfs_subvolume_create"; "/test2"]; ["btrfs_subvolume_create"; "/dir/test3"]; - ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test5"; "true"]]), [] + ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test5"; "true"; "NOARG"]; + ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test6"; ""; "0/1000"]]), [] ]; shortdesc = "create a writable btrfs snapshot"; longdesc = "\
This needs the documentation for the "qgroupid" optarg in the longdesc. LGTM otherwise. -- Pino Toscano
Parameter `qgroupid' is for adding the created subvolume to a qgroup. Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- daemon/btrfs.c | 10 +++++++++- generator/actions.ml | 15 ++++++++------- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/daemon/btrfs.c b/daemon/btrfs.c index a20afca..0f525fa 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -291,7 +291,7 @@ do_btrfs_subvolume_delete (const char *subvolume) } int -do_btrfs_subvolume_create (const char *dest) +do_btrfs_subvolume_create (const char *dest, const char *qgroupid) { const size_t MAX_ARGS = 64; const char *argv[MAX_ARGS]; @@ -309,6 +309,14 @@ do_btrfs_subvolume_create (const char *dest) ADD_ARG (argv, i, str_btrfs); ADD_ARG (argv, i, "subvolume"); ADD_ARG (argv, i, "create"); + + /* Optional arguments. */ + if (optargs_bitmask & GUESTFS_BTRFS_SUBVOLUME_CREATE_QGROUPID_BITMASK) { + ADD_ARG (argv, i, "-i"); + ADD_ARG (argv, i, qgroupid); + } + + ADD_ARG (argv, i, dest_buf); ADD_ARG (argv, i, NULL); diff --git a/generator/actions.ml b/generator/actions.ml index 30b839c..1c1fcff 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -10225,9 +10225,9 @@ See C<guestfs_get_e2generation>." }; [["mkfs_btrfs"; "/dev/sda1"; ""; ""; "NOARG"; ""; "NOARG"; "NOARG"; ""; ""]; ["mount"; "/dev/sda1"; "/"]; ["mkdir"; "/dir"]; - ["btrfs_subvolume_create"; "/test1"]; - ["btrfs_subvolume_create"; "/test2"]; - ["btrfs_subvolume_create"; "/dir/test3"]; + ["btrfs_subvolume_create"; "/test1"; "NOARG"]; + ["btrfs_subvolume_create"; "/test2"; "NOARG"]; + ["btrfs_subvolume_create"; "/dir/test3"; "NOARG"]; ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test5"; "true"; "NOARG"]; ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test6"; ""; "0/1000"]]), [] ]; @@ -10246,7 +10246,7 @@ of the snapshot, in the form C</path/to/dest/name>." }; InitPartition, Always, TestRun ( [["mkfs_btrfs"; "/dev/sda1"; ""; ""; "NOARG"; ""; "NOARG"; "NOARG"; ""; ""]; ["mount"; "/dev/sda1"; "/"]; - ["btrfs_subvolume_create"; "/test1"]; + ["btrfs_subvolume_create"; "/test1"; "NOARG"]; ["btrfs_subvolume_delete"; "/test1"]]), [] ]; shortdesc = "delete a btrfs subvolume or snapshot"; @@ -10255,13 +10255,14 @@ Delete the named btrfs subvolume or snapshot." }; { defaults with name = "btrfs_subvolume_create"; - style = RErr, [Pathname "dest"], []; + style = RErr, [Pathname "dest"], [OString "qgroupid"]; proc_nr = Some 324; optional = Some "btrfs"; camel_name = "BTRFSSubvolumeCreate"; shortdesc = "create a btrfs subvolume"; longdesc = "\ Create a btrfs subvolume. The C<dest> argument is the destination -directory and the name of the subvolume, in the form C</path/to/dest/name>." }; +directory and the name of the subvolume, in the form C</path/to/dest/name>. +The C<qgroupid> adds the newly created subvolume to a qgroup." }; { defaults with name = "btrfs_subvolume_list"; @@ -10295,7 +10296,7 @@ get a list of subvolumes." }; InitPartition, Always, TestRun ( [["mkfs_btrfs"; "/dev/sda1"; ""; ""; "NOARG"; ""; "NOARG"; "NOARG"; ""; ""]; ["mount"; "/dev/sda1"; "/"]; - ["btrfs_subvolume_create"; "/test1"]; + ["btrfs_subvolume_create"; "/test1"; "NOARG"]; ["btrfs_filesystem_sync"; "/test1"]; ["btrfs_filesystem_balance"; "/test1"]]), [] ]; -- 1.9.3
On Friday 21 November 2014 13:17:58 Hu Tao wrote:
Parameter `qgroupid' is for adding the created subvolume to a qgroup.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- daemon/btrfs.c | 10 +++++++++- generator/actions.ml | 15 ++++++++------- 2 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c index a20afca..0f525fa 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -291,7 +291,7 @@ do_btrfs_subvolume_delete (const char *subvolume) }
int -do_btrfs_subvolume_create (const char *dest) +do_btrfs_subvolume_create (const char *dest, const char *qgroupid) { const size_t MAX_ARGS = 64; const char *argv[MAX_ARGS]; @@ -309,6 +309,14 @@ do_btrfs_subvolume_create (const char *dest) ADD_ARG (argv, i, str_btrfs); ADD_ARG (argv, i, "subvolume"); ADD_ARG (argv, i, "create"); + + /* Optional arguments. */ + if (optargs_bitmask & GUESTFS_BTRFS_SUBVOLUME_CREATE_QGROUPID_BITMASK) { + ADD_ARG (argv, i, "-i"); + ADD_ARG (argv, i, qgroupid); + } + + ADD_ARG (argv, i, dest_buf); ADD_ARG (argv, i, NULL);
diff --git a/generator/actions.ml b/generator/actions.ml index 30b839c..1c1fcff 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -10225,9 +10225,9 @@ See C<guestfs_get_e2generation>." }; [["mkfs_btrfs"; "/dev/sda1"; ""; ""; "NOARG"; ""; "NOARG"; "NOARG"; ""; ""]; ["mount"; "/dev/sda1"; "/"]; ["mkdir"; "/dir"]; - ["btrfs_subvolume_create"; "/test1"]; - ["btrfs_subvolume_create"; "/test2"]; - ["btrfs_subvolume_create"; "/dir/test3"]; + ["btrfs_subvolume_create"; "/test1"; "NOARG"]; + ["btrfs_subvolume_create"; "/test2"; "NOARG"]; + ["btrfs_subvolume_create"; "/dir/test3"; "NOARG"]; ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test5"; "true"; "NOARG"]; ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test6"; ""; "0/1000"]]), [] ]; @@ -10246,7 +10246,7 @@ of the snapshot, in the form C</path/to/dest/name>." }; InitPartition, Always, TestRun ( [["mkfs_btrfs"; "/dev/sda1"; ""; ""; "NOARG"; ""; "NOARG"; "NOARG"; ""; ""]; ["mount"; "/dev/sda1"; "/"]; - ["btrfs_subvolume_create"; "/test1"]; + ["btrfs_subvolume_create"; "/test1"; "NOARG"]; ["btrfs_subvolume_delete"; "/test1"]]), [] ]; shortdesc = "delete a btrfs subvolume or snapshot"; @@ -10255,13 +10255,14 @@ Delete the named btrfs subvolume or snapshot." };
{ defaults with name = "btrfs_subvolume_create"; - style = RErr, [Pathname "dest"], []; + style = RErr, [Pathname "dest"], [OString "qgroupid"]; proc_nr = Some 324; optional = Some "btrfs"; camel_name = "BTRFSSubvolumeCreate"; shortdesc = "create a btrfs subvolume";
Like in patch 2, you need once_had_no_optargs = true for this as well.
longdesc = "\ Create a btrfs subvolume. The C<dest> argument is the destination -directory and the name of the subvolume, in the form C</path/to/dest/name>." }; +directory and the name of the subvolume, in the form C</path/to/dest/name>. +The C<qgroupid> adds the newly created subvolume to a qgroup." };
"The optional C<qgroupid> parameter represents the qgroup which the newly created subvolume should be added to." or something like that, I'm not an English native speaker either... -- Pino Toscano
On Fri, Nov 21, 2014 at 11:46:37AM +0100, Pino Toscano wrote:
On Friday 21 November 2014 13:17:58 Hu Tao wrote:
Parameter `qgroupid' is for adding the created subvolume to a qgroup.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- daemon/btrfs.c | 10 +++++++++- generator/actions.ml | 15 ++++++++------- 2 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c index a20afca..0f525fa 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -291,7 +291,7 @@ do_btrfs_subvolume_delete (const char *subvolume) }
int -do_btrfs_subvolume_create (const char *dest) +do_btrfs_subvolume_create (const char *dest, const char *qgroupid) { const size_t MAX_ARGS = 64; const char *argv[MAX_ARGS]; @@ -309,6 +309,14 @@ do_btrfs_subvolume_create (const char *dest) ADD_ARG (argv, i, str_btrfs); ADD_ARG (argv, i, "subvolume"); ADD_ARG (argv, i, "create"); + + /* Optional arguments. */ + if (optargs_bitmask & GUESTFS_BTRFS_SUBVOLUME_CREATE_QGROUPID_BITMASK) { + ADD_ARG (argv, i, "-i"); + ADD_ARG (argv, i, qgroupid); + } + + ADD_ARG (argv, i, dest_buf); ADD_ARG (argv, i, NULL);
diff --git a/generator/actions.ml b/generator/actions.ml index 30b839c..1c1fcff 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -10225,9 +10225,9 @@ See C<guestfs_get_e2generation>." }; [["mkfs_btrfs"; "/dev/sda1"; ""; ""; "NOARG"; ""; "NOARG"; "NOARG"; ""; ""]; ["mount"; "/dev/sda1"; "/"]; ["mkdir"; "/dir"]; - ["btrfs_subvolume_create"; "/test1"]; - ["btrfs_subvolume_create"; "/test2"]; - ["btrfs_subvolume_create"; "/dir/test3"]; + ["btrfs_subvolume_create"; "/test1"; "NOARG"]; + ["btrfs_subvolume_create"; "/test2"; "NOARG"]; + ["btrfs_subvolume_create"; "/dir/test3"; "NOARG"]; ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test5"; "true"; "NOARG"]; ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test6"; ""; "0/1000"]]), [] ]; @@ -10246,7 +10246,7 @@ of the snapshot, in the form C</path/to/dest/name>." }; InitPartition, Always, TestRun ( [["mkfs_btrfs"; "/dev/sda1"; ""; ""; "NOARG"; ""; "NOARG"; "NOARG"; ""; ""]; ["mount"; "/dev/sda1"; "/"]; - ["btrfs_subvolume_create"; "/test1"]; + ["btrfs_subvolume_create"; "/test1"; "NOARG"]; ["btrfs_subvolume_delete"; "/test1"]]), [] ]; shortdesc = "delete a btrfs subvolume or snapshot"; @@ -10255,13 +10255,14 @@ Delete the named btrfs subvolume or snapshot." };
{ defaults with name = "btrfs_subvolume_create"; - style = RErr, [Pathname "dest"], []; + style = RErr, [Pathname "dest"], [OString "qgroupid"]; proc_nr = Some 324; optional = Some "btrfs"; camel_name = "BTRFSSubvolumeCreate"; shortdesc = "create a btrfs subvolume";
Like in patch 2, you need once_had_no_optargs = true for this as well.
Yes.
longdesc = "\ Create a btrfs subvolume. The C<dest> argument is the destination -directory and the name of the subvolume, in the form C</path/to/dest/name>." }; +directory and the name of the subvolume, in the form C</path/to/dest/name>. +The C<qgroupid> adds the newly created subvolume to a qgroup." };
"The optional C<qgroupid> parameter represents the qgroup which the newly created subvolume should be added to."
"...will be added to." since it is a optional parameter? Regards, Hu
or something like that, I'm not an English native speaker either...
-- Pino Toscano
On Mon, Nov 24, 2014 at 10:26:50AM +0800, Hu Tao wrote:
On Fri, Nov 21, 2014 at 11:46:37AM +0100, Pino Toscano wrote:
On Friday 21 November 2014 13:17:58 Hu Tao wrote:
Parameter `qgroupid' is for adding the created subvolume to a qgroup.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- daemon/btrfs.c | 10 +++++++++- generator/actions.ml | 15 ++++++++------- 2 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c index a20afca..0f525fa 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -291,7 +291,7 @@ do_btrfs_subvolume_delete (const char *subvolume) }
int -do_btrfs_subvolume_create (const char *dest) +do_btrfs_subvolume_create (const char *dest, const char *qgroupid) { const size_t MAX_ARGS = 64; const char *argv[MAX_ARGS]; @@ -309,6 +309,14 @@ do_btrfs_subvolume_create (const char *dest) ADD_ARG (argv, i, str_btrfs); ADD_ARG (argv, i, "subvolume"); ADD_ARG (argv, i, "create"); + + /* Optional arguments. */ + if (optargs_bitmask & GUESTFS_BTRFS_SUBVOLUME_CREATE_QGROUPID_BITMASK) { + ADD_ARG (argv, i, "-i"); + ADD_ARG (argv, i, qgroupid); + } + + ADD_ARG (argv, i, dest_buf); ADD_ARG (argv, i, NULL);
diff --git a/generator/actions.ml b/generator/actions.ml index 30b839c..1c1fcff 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -10225,9 +10225,9 @@ See C<guestfs_get_e2generation>." }; [["mkfs_btrfs"; "/dev/sda1"; ""; ""; "NOARG"; ""; "NOARG"; "NOARG"; ""; ""]; ["mount"; "/dev/sda1"; "/"]; ["mkdir"; "/dir"]; - ["btrfs_subvolume_create"; "/test1"]; - ["btrfs_subvolume_create"; "/test2"]; - ["btrfs_subvolume_create"; "/dir/test3"]; + ["btrfs_subvolume_create"; "/test1"; "NOARG"]; + ["btrfs_subvolume_create"; "/test2"; "NOARG"]; + ["btrfs_subvolume_create"; "/dir/test3"; "NOARG"]; ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test5"; "true"; "NOARG"]; ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test6"; ""; "0/1000"]]), [] ]; @@ -10246,7 +10246,7 @@ of the snapshot, in the form C</path/to/dest/name>." }; InitPartition, Always, TestRun ( [["mkfs_btrfs"; "/dev/sda1"; ""; ""; "NOARG"; ""; "NOARG"; "NOARG"; ""; ""]; ["mount"; "/dev/sda1"; "/"]; - ["btrfs_subvolume_create"; "/test1"]; + ["btrfs_subvolume_create"; "/test1"; "NOARG"]; ["btrfs_subvolume_delete"; "/test1"]]), [] ]; shortdesc = "delete a btrfs subvolume or snapshot"; @@ -10255,13 +10255,14 @@ Delete the named btrfs subvolume or snapshot." };
{ defaults with name = "btrfs_subvolume_create"; - style = RErr, [Pathname "dest"], []; + style = RErr, [Pathname "dest"], [OString "qgroupid"]; proc_nr = Some 324; optional = Some "btrfs"; camel_name = "BTRFSSubvolumeCreate"; shortdesc = "create a btrfs subvolume";
Like in patch 2, you need once_had_no_optargs = true for this as well.
Yes.
longdesc = "\ Create a btrfs subvolume. The C<dest> argument is the destination -directory and the name of the subvolume, in the form C</path/to/dest/name>." }; +directory and the name of the subvolume, in the form C</path/to/dest/name>. +The C<qgroupid> adds the newly created subvolume to a qgroup." };
"The optional C<qgroupid> parameter represents the qgroup which the newly created subvolume should be added to."
"...will be added to." since it is a optional parameter?
I changed it into "The optional parameter C<qgroupid> represents the qgroup which the newly created subvolume will be added to.". How does that sound?
Regards, Hu
or something like that, I'm not an English native speaker either...
-- Pino Toscano
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
On Monday 24 November 2014 10:39:36 Hu Tao wrote:
On Mon, Nov 24, 2014 at 10:26:50AM +0800, Hu Tao wrote:
On Fri, Nov 21, 2014 at 11:46:37AM +0100, Pino Toscano wrote:
On Friday 21 November 2014 13:17:58 Hu Tao wrote:
Parameter `qgroupid' is for adding the created subvolume to a qgroup.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> ---
daemon/btrfs.c | 10 +++++++++- generator/actions.ml | 15 ++++++++------- 2 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c index a20afca..0f525fa 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -291,7 +291,7 @@ do_btrfs_subvolume_delete (const char *subvolume)
}
int
-do_btrfs_subvolume_create (const char *dest) +do_btrfs_subvolume_create (const char *dest, const char *qgroupid)
{
const size_t MAX_ARGS = 64; const char *argv[MAX_ARGS];
@@ -309,6 +309,14 @@ do_btrfs_subvolume_create (const char *dest)
ADD_ARG (argv, i, str_btrfs); ADD_ARG (argv, i, "subvolume"); ADD_ARG (argv, i, "create");
+ + /* Optional arguments. */ + if (optargs_bitmask & GUESTFS_BTRFS_SUBVOLUME_CREATE_QGROUPID_BITMASK) { + ADD_ARG (argv, i, "-i"); + ADD_ARG (argv, i, qgroupid); + } + +
ADD_ARG (argv, i, dest_buf); ADD_ARG (argv, i, NULL);
diff --git a/generator/actions.ml b/generator/actions.ml index 30b839c..1c1fcff 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -10225,9 +10225,9 @@ See C<guestfs_get_e2generation>." };
[["mkfs_btrfs"; "/dev/sda1"; ""; ""; "NOARG"; ""; "NOARG"; "NOARG"; ""; ""];> > > ["mount"; "/dev/sda1"; "/"]; ["mkdir"; "/dir"];
- ["btrfs_subvolume_create"; "/test1"]; - ["btrfs_subvolume_create"; "/test2"]; - ["btrfs_subvolume_create"; "/dir/test3"]; + ["btrfs_subvolume_create"; "/test1"; "NOARG"]; + ["btrfs_subvolume_create"; "/test2"; "NOARG"]; + ["btrfs_subvolume_create"; "/dir/test3"; "NOARG"];
["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test5"; "true"; "NOARG"]; ["btrfs_subvolume_snapshot"; "/dir/test3"; "/dir/test6"; ""; "0/1000"]]), []> > > ];
@@ -10246,7 +10246,7 @@ of the snapshot, in the form C</path/to/dest/name>." };> > > InitPartition, Always, TestRun (
[["mkfs_btrfs"; "/dev/sda1"; ""; ""; "NOARG"; ""; "NOARG"; "NOARG"; ""; ""];> > > ["mount"; "/dev/sda1"; "/"];
- ["btrfs_subvolume_create"; "/test1"]; + ["btrfs_subvolume_create"; "/test1"; "NOARG"];
["btrfs_subvolume_delete"; "/test1"]]), []
]; shortdesc = "delete a btrfs subvolume or snapshot";
@@ -10255,13 +10255,14 @@ Delete the named btrfs subvolume or snapshot." };> > > { defaults with
name = "btrfs_subvolume_create";
- style = RErr, [Pathname "dest"], []; + style = RErr, [Pathname "dest"], [OString "qgroupid"];
proc_nr = Some 324; optional = Some "btrfs"; camel_name = "BTRFSSubvolumeCreate"; shortdesc = "create a btrfs subvolume";
Like in patch 2, you need once_had_no_optargs = true for this as well.> Yes.
longdesc = "\
Create a btrfs subvolume. The C<dest> argument is the destination
-directory and the name of the subvolume, in the form C</path/to/dest/name>." }; +directory and the name of the subvolume, in the form C</path/to/dest/name>. +The C<qgroupid> adds the newly created subvolume to a qgroup." };> >
"The optional C<qgroupid> parameter represents the qgroup which the newly created subvolume should be added to."
"...will be added to." since it is a optional parameter?
I changed it into "The optional parameter C<qgroupid> represents the qgroup which the newly created subvolume will be added to.". How does that sound?
Sounds okay to me. Thanks, -- Pino Toscano
btrfs_subvolume_get_default is for getting the default subvolume of a btrfs filesystem. Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- daemon/btrfs.c | 38 ++++++++++++++++++++++++++++++++++++++ generator/actions.ml | 9 +++++++++ src/MAX_PROC_NR | 2 +- 3 files changed, 48 insertions(+), 1 deletion(-) diff --git a/daemon/btrfs.c b/daemon/btrfs.c index 0f525fa..e179b57 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -549,6 +549,44 @@ do_btrfs_subvolume_set_default (int64_t id, const char *fs) } int +do_btrfs_subvolume_get_default (const char *fs) +{ + const size_t MAX_ARGS = 64; + const char *argv[MAX_ARGS]; + size_t i = 0; + CLEANUP_FREE char *fs_buf = NULL; + CLEANUP_FREE char *err = NULL; + CLEANUP_FREE char *out = NULL; + int r; + + fs_buf = sysroot_path (fs); + if (fs_buf == NULL) { + reply_with_perror ("malloc"); + return -1; + } + + ADD_ARG (argv, i, str_btrfs); + ADD_ARG (argv, i, "subvolume"); + ADD_ARG (argv, i, "get-default"); + ADD_ARG (argv, i, fs_buf); + ADD_ARG (argv, i, NULL); + + r = commandv (&out, &err, argv); + fprintf(stderr, "%s\n", out); + if (r == -1) { + reply_with_error ("%s: %s", fs, err); + return -1; + } + r = -1; + if (sscanf (out, "ID %" SCNi32, &r) != 1) { + reply_with_error ("%s: expected output, but got nothing", argv[0]); + return -1; + } + + return r; +} + +int do_btrfs_filesystem_sync (const char *fs) { const size_t MAX_ARGS = 64; diff --git a/generator/actions.ml b/generator/actions.ml index 1c1fcff..cf96039 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -12005,6 +12005,15 @@ This is the same as the C<lstat(2)> system call." }; longdesc = "\ This is the internal call which implements C<guestfs_lstatnslist>." }; + { defaults with + name = "btrfs_subvolume_get_default"; + style = RInt "id", [Pathname "mountpoint"], []; + proc_nr = Some 424; + optional = Some "btrfs"; camel_name = "BTRFSSubvolumeGetDefault"; + shortdesc = "get the default subvolume or snapshot of a filesystem"; + longdesc = "\ +Get the default subvolume or snapshot of a filesystem mounted at C<mountpoint>." }; + ] (* Non-API meta-commands available only in guestfish. diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR index 5721413..9524ef4 100644 --- a/src/MAX_PROC_NR +++ b/src/MAX_PROC_NR @@ -1 +1 @@ -423 +424 -- 1.9.3
On Friday 21 November 2014 13:17:59 Hu Tao wrote:
btrfs_subvolume_get_default is for getting the default subvolume of a btrfs filesystem.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- daemon/btrfs.c | 38 ++++++++++++++++++++++++++++++++++++++ generator/actions.ml | 9 +++++++++ src/MAX_PROC_NR | 2 +- 3 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c index 0f525fa..e179b57 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -549,6 +549,44 @@ do_btrfs_subvolume_set_default (int64_t id, const char *fs) }
int +do_btrfs_subvolume_get_default (const char *fs) +{ + const size_t MAX_ARGS = 64; + const char *argv[MAX_ARGS]; + size_t i = 0; + CLEANUP_FREE char *fs_buf = NULL; + CLEANUP_FREE char *err = NULL; + CLEANUP_FREE char *out = NULL; + int r; + + fs_buf = sysroot_path (fs); + if (fs_buf == NULL) { + reply_with_perror ("malloc"); + return -1; + } + + ADD_ARG (argv, i, str_btrfs); + ADD_ARG (argv, i, "subvolume"); + ADD_ARG (argv, i, "get-default"); + ADD_ARG (argv, i, fs_buf); + ADD_ARG (argv, i, NULL); + + r = commandv (&out, &err, argv); + fprintf(stderr, "%s\n", out);
This looks like a leftover? Otherwise, print it only when "debug" is enabled (if (debug) ...).
+ if (r == -1) { + reply_with_error ("%s: %s", fs, err); + return -1; + } + r = -1;
Most probably it is not needed to manually reset r.
+ if (sscanf (out, "ID %" SCNi32, &r) != 1) {
It would be simplier to just use xstrtol to parse the output.
+ reply_with_error ("%s: expected output, but got nothing", argv[0]);
More than "nothing", "not a number". I'd also append the whole output here.
diff --git a/generator/actions.ml b/generator/actions.ml index 1c1fcff..cf96039 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -12005,6 +12005,15 @@ This is the same as the C<lstat(2)> system call." }; longdesc = "\ This is the internal call which implements C<guestfs_lstatnslist>." };
+ { defaults with + name = "btrfs_subvolume_get_default"; + style = RInt "id", [Pathname "mountpoint"], []; + proc_nr = Some 424; + optional = Some "btrfs"; camel_name = "BTRFSSubvolumeGetDefault"; + shortdesc = "get the default subvolume or snapshot of a filesystem"; + longdesc = "\ +Get the default subvolume or snapshot of a filesystem mounted at C<mountpoint>." }; + ]
(* Non-API meta-commands available only in guestfish. diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR index 5721413..9524ef4 100644 --- a/src/MAX_PROC_NR +++ b/src/MAX_PROC_NR @@ -1 +1 @@ -423 +424 -- 1.9.3
-- Pino Toscano
On Fri, Nov 21, 2014 at 12:11:36PM +0100, Pino Toscano wrote:
On Friday 21 November 2014 13:17:59 Hu Tao wrote:
btrfs_subvolume_get_default is for getting the default subvolume of a btrfs filesystem.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- daemon/btrfs.c | 38 ++++++++++++++++++++++++++++++++++++++ generator/actions.ml | 9 +++++++++ src/MAX_PROC_NR | 2 +- 3 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c index 0f525fa..e179b57 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -549,6 +549,44 @@ do_btrfs_subvolume_set_default (int64_t id, const char *fs) }
int +do_btrfs_subvolume_get_default (const char *fs) +{ + const size_t MAX_ARGS = 64; + const char *argv[MAX_ARGS]; + size_t i = 0; + CLEANUP_FREE char *fs_buf = NULL; + CLEANUP_FREE char *err = NULL; + CLEANUP_FREE char *out = NULL; + int r; + + fs_buf = sysroot_path (fs); + if (fs_buf == NULL) { + reply_with_perror ("malloc"); + return -1; + } + + ADD_ARG (argv, i, str_btrfs); + ADD_ARG (argv, i, "subvolume"); + ADD_ARG (argv, i, "get-default"); + ADD_ARG (argv, i, fs_buf); + ADD_ARG (argv, i, NULL); + + r = commandv (&out, &err, argv); + fprintf(stderr, "%s\n", out);
This looks like a leftover? Otherwise, print it only when "debug" is enabled (if (debug) ...).
Oops, I will just remove the line.
+ if (r == -1) { + reply_with_error ("%s: %s", fs, err); + return -1; + } + r = -1;
Most probably it is not needed to manually reset r.
+ if (sscanf (out, "ID %" SCNi32, &r) != 1) {
It would be simplier to just use xstrtol to parse the output.
Then we should skip "ID " firstly.
+ reply_with_error ("%s: expected output, but got nothing", argv[0]);
More than "nothing", "not a number". I'd also append the whole output here.
Sounds good.
diff --git a/generator/actions.ml b/generator/actions.ml index 1c1fcff..cf96039 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -12005,6 +12005,15 @@ This is the same as the C<lstat(2)> system call." }; longdesc = "\ This is the internal call which implements C<guestfs_lstatnslist>." };
+ { defaults with + name = "btrfs_subvolume_get_default"; + style = RInt "id", [Pathname "mountpoint"], []; + proc_nr = Some 424; + optional = Some "btrfs"; camel_name = "BTRFSSubvolumeGetDefault"; + shortdesc = "get the default subvolume or snapshot of a filesystem"; + longdesc = "\ +Get the default subvolume or snapshot of a filesystem mounted at C<mountpoint>." }; + ]
(* Non-API meta-commands available only in guestfish. diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR index 5721413..9524ef4 100644 --- a/src/MAX_PROC_NR +++ b/src/MAX_PROC_NR @@ -1 +1 @@ -423 +424 -- 1.9.3
-- Pino Toscano
On Monday 24 November 2014 13:44:11 Hu Tao wrote:
+ if (sscanf (out, "ID %" SCNi32, &r) != 1) {
It would be simplier to just use xstrtol to parse the output.
Then we should skip "ID " firstly.
Right (got focused on the integer parsing, and lost track of the "ID " prefix). Then I guess sscanf could stay as you wrote it. -- Pino Toscano
btrfs_subvolume_show shows the detailed information of a subvolume or snapshot. Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- daemon/btrfs.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++++++ generator/actions.ml | 9 +++ src/MAX_PROC_NR | 2 +- 3 files changed, 177 insertions(+), 1 deletion(-) diff --git a/daemon/btrfs.c b/daemon/btrfs.c index e179b57..36ba588 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -29,6 +29,7 @@ #include "actions.h" #include "optgroups.h" #include "xstrtol.h" +#include "c-ctype.h" GUESTFSD_EXT_CMD(str_btrfs, btrfs); GUESTFSD_EXT_CMD(str_btrfstune, btrfstune); @@ -813,3 +814,169 @@ do_btrfs_fsck (const char *device, int64_t superblock, int repair) return 0; } + +char **do_btrfs_subvolume_show (const char *subvolume) +{ + const size_t MAX_ARGS = 64; + const char *argv[MAX_ARGS]; + size_t i = 0; + CLEANUP_FREE char *subvolume_buf = NULL; + CLEANUP_FREE char *err = NULL; + CLEANUP_FREE char *out = NULL; + char *p, *pend, *colon; + DECLARE_STRINGSBUF (ret); + int r; + + subvolume_buf = sysroot_path (subvolume); + if (subvolume_buf == NULL) { + reply_with_perror ("malloc"); + return NULL; + } + + ADD_ARG (argv, i, str_btrfs); + ADD_ARG (argv, i, "subvolume"); + ADD_ARG (argv, i, "show"); + ADD_ARG (argv, i, subvolume_buf); + ADD_ARG (argv, i, NULL); + + r = commandv (&out, &err, argv); + if (r == -1) { + reply_with_error ("%s: %s", subvolume, err); + return NULL; + } + + /* Output is: + * + * / + * Name: root + * uuid: c875169e-cf4e-a04d-9959-b667dec36234 + * Parent uuid: - + * Creation time: 2014-11-13 10:13:08 + * Object ID: 256 + * Generation (Gen): 6579 + * Gen at creation: 5 + * Parent: 5 + * Top Level: 5 + * Flags: - + * Snapshot(s): + * snapshots/test1 + * snapshots/test2 + * snapshots/test3 + * + */ + + p = out; + + p = strchr (p, '\n'); + if (p) { + *p = '\0'; + p++; + } + else { + reply_with_error ("truncated output"); + return NULL; + } + + /* If the path is the btrfs root, `btrfs subvolume show' reports: + * <path> is btrfs root + */ + if (strstr(out, "is btrfs root") != NULL) { + reply_with_error ("%s is btrfs root", subvolume); + return NULL; + } + + /* The first line is the path of the subvolume. */ + if (add_string (&ret, strdup("path")) == -1) { + return NULL; + } + if (add_string (&ret, out) == -1) { + return NULL; + } + + /* Read the lines and split into "key: value". */ + while (*p) { + /* leading spaces and tabs */ + while (*p && c_isspace (*p)) p++; + + pend = strchrnul (p, '\n'); + if (*pend == '\n') { + *pend = '\0'; + pend++; + } + + if (!*p) continue; + + colon = strchr (p, ':'); + if (colon) { + *colon = '\0'; + + /* snapshot is special, see the output above */ + if (strncmp(p, "Snapshot", sizeof("Snapshot") - 1) == 0) { + char *ss = NULL; + + if (add_string (&ret, p) == -1) { + return NULL; + } + + /* each line is the path of a snapshot. */ + p = pend; + while (*p && strchr(p, ':') == NULL) { + while (*p && c_isspace (*p)) p++; + pend = strchrnul (p, '\n'); + if (*pend == '\n') { + *pend = '\0'; + pend++; + } + if (ss) { + ss = realloc(ss, strlen(ss) + strlen(p)); + strcat(ss, ","); + strcat(ss, p); + } else { + ss = strdup(p); + } + + p = pend; + } + + if (ss) { + if (add_string (&ret, ss) == -1) { + free(ss); + return NULL; + } + } + else { + if (add_string (&ret, strdup("")) == -1) { + return NULL; + } + } + + continue; + } + + do { colon++; } while (*colon && c_isspace (*colon)); + + if (add_string (&ret, p) == -1) { + return NULL; + } + if (add_string (&ret, colon) == -1) { + return NULL; + } + } + else { + if (add_string (&ret, p) == -1) { + return NULL; + } + if (add_string (&ret, "") == -1) { + return NULL; + } + } + + p = pend; + } + + if (end_stringsbuf (&ret) == -1) + return NULL; + + return ret.argv; + +} diff --git a/generator/actions.ml b/generator/actions.ml index cf96039..24d3ecc 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -12014,6 +12014,15 @@ This is the internal call which implements C<guestfs_lstatnslist>." }; longdesc = "\ Get the default subvolume or snapshot of a filesystem mounted at C<mountpoint>." }; + { defaults with + name = "btrfs_subvolume_show"; + style = RHashtable "btrfssubvolumeinfo", [Pathname "subvolume"], []; + proc_nr = Some 425; + optional = Some "btrfs"; camel_name = "BTRFSSubvolumeShow"; + shortdesc = "show detailed information of the subvolume"; + longdesc = "\ +show detailed information of the subvolume." }; + ] (* Non-API meta-commands available only in guestfish. diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR index 9524ef4..5e4a522 100644 --- a/src/MAX_PROC_NR +++ b/src/MAX_PROC_NR @@ -1 +1 @@ -424 +425 -- 1.9.3
On Friday 21 November 2014 13:18:00 Hu Tao wrote:
btrfs_subvolume_show shows the detailed information of a subvolume or snapshot.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- daemon/btrfs.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++++++ generator/actions.ml | 9 +++ src/MAX_PROC_NR | 2 +- 3 files changed, 177 insertions(+), 1 deletion(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c index e179b57..36ba588 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -29,6 +29,7 @@ #include "actions.h" #include "optgroups.h" #include "xstrtol.h" +#include "c-ctype.h"
GUESTFSD_EXT_CMD(str_btrfs, btrfs); GUESTFSD_EXT_CMD(str_btrfstune, btrfstune); @@ -813,3 +814,169 @@ do_btrfs_fsck (const char *device, int64_t superblock, int repair)
return 0; } + +char **do_btrfs_subvolume_show (const char *subvolume) +{ + const size_t MAX_ARGS = 64; + const char *argv[MAX_ARGS]; + size_t i = 0; + CLEANUP_FREE char *subvolume_buf = NULL; + CLEANUP_FREE char *err = NULL; + CLEANUP_FREE char *out = NULL; + char *p, *pend, *colon; + DECLARE_STRINGSBUF (ret); + int r; + + subvolume_buf = sysroot_path (subvolume); + if (subvolume_buf == NULL) { + reply_with_perror ("malloc"); + return NULL; + } + + ADD_ARG (argv, i, str_btrfs); + ADD_ARG (argv, i, "subvolume"); + ADD_ARG (argv, i, "show"); + ADD_ARG (argv, i, subvolume_buf); + ADD_ARG (argv, i, NULL); + + r = commandv (&out, &err, argv); + if (r == -1) { + reply_with_error ("%s: %s", subvolume, err); + return NULL; + } + + /* Output is: + * + * / + * Name: root + * uuid: c875169e-cf4e-a04d-9959-b667dec36234 + * Parent uuid: - + * Creation time: 2014-11-13 10:13:08 + * Object ID: 256 + * Generation (Gen): 6579 + * Gen at creation: 5 + * Parent: 5 + * Top Level: 5 + * Flags: - + * Snapshot(s): + * snapshots/test1 + * snapshots/test2 + * snapshots/test3 + * + */ + + p = out; + + p = strchr (p, '\n'); + if (p) { + *p = '\0'; + p++; + } + else {
Minor style note: } else { (also in other parts of this patch)
+ reply_with_error ("truncated output");
I'd print out anyway, to ease debugging failures a bit.
+ return NULL; + } + + /* If the path is the btrfs root, `btrfs subvolume show' reports: + * <path> is btrfs root + */ + if (strstr(out, "is btrfs root") != NULL) {
Take care of adding a space before parenthesis (also in other parts of this patch).
+ reply_with_error ("%s is btrfs root", subvolume); + return NULL; + } + + /* The first line is the path of the subvolume. */ + if (add_string (&ret, strdup("path")) == -1) {
add_string duplicates the string, so you don't need to do that manually.
+ return NULL; + } + if (add_string (&ret, out) == -1) { + return NULL; + } + + /* Read the lines and split into "key: value". */ + while (*p) { + /* leading spaces and tabs */ + while (*p && c_isspace (*p)) p++;
Properly indent such lines, so: while (*p && c_isspace (*p)) p++; (also, a minor optimization is to use ++p instead of p++, as it can avoid a temporary value)
+ + pend = strchrnul (p, '\n'); + if (*pend == '\n') { + *pend = '\0'; + pend++; + } + + if (!*p) continue; + + colon = strchr (p, ':'); + if (colon) { + *colon = '\0'; + + /* snapshot is special, see the output above */ + if (strncmp(p, "Snapshot", sizeof("Snapshot") - 1) == 0) {
Please use the STR* macros we have in src/guestfs-internal-all.h. Also, most probably it is better to match "Snapshot(s)", so if in the future a new field starting with "Snapshot" is added then it won't be mistaken for this multiline one.
+ char *ss = NULL; + + if (add_string (&ret, p) == -1) { + return NULL; + }
Single statements don't need curly brackets (also in other parts of the patch).
+ + /* each line is the path of a snapshot. */ + p = pend; + while (*p && strchr(p, ':') == NULL) {
This will break once the output will have fields after Snapshot, as strchr(p, ':') will return a non-null value in one of the lines following the first line after the Snapshot one.
+ while (*p && c_isspace (*p)) p++; + pend = strchrnul (p, '\n'); + if (*pend == '\n') { + *pend = '\0'; + pend++; + }
I see this code repeated already; I think it would be better to create a small helper function like: char *analyze_line (char *p, char **field, char **value) which would - destructively update p (setting null bytes, just like it is done now) - return in field the start of the field text - return in value, if present, the start of the value (after the colon) - return the pointer to the first character of the new line (or null, if none following) this way it is IMHO easier to iterate over the lines, and also to detect whether a line contains a field+value or just text.
+ if (ss) { + ss = realloc(ss, strlen(ss) + strlen(p));
If realloc fails, the return value is null; this means that the following will leak ss in that case. Also, sounds like you are missing "," in the length of the string?
+ strcat(ss, ",");
Just save the length of ss before the realloc, and you can simply do ss[old_length_of_ss] = ',';
+ strcat(ss, p);
You can easily use memcpy here: memcpy (ss + old_length_of_ss + 1, p, strlen (p));
+ } else { + ss = strdup(p);
Missing check for failed strdup.
+ } + + p = pend; + } + + if (ss) { + if (add_string (&ret, ss) == -1) {
This needs to be add_string_nodup.
+ free(ss);
No need to free ss here; theoretically add_string will take care of it, be it added to the stringbuf or freed on error.
+ return NULL; + } + } + else { + if (add_string (&ret, strdup("")) == -1) { + return NULL; + } + }
If there are no elements for "Snapshot(s)", then it would be better to just not add an empty hash element.
+ + continue; + } + + do { colon++; } while (*colon && c_isspace (*colon)); + if (add_string (&ret, p) == -1) { + return NULL; + } + if (add_string (&ret, colon) == -1) { + return NULL; + }
At least from the example output added as comment, it seems that "not available" values might be "-" -- it'd just not add those, since as users of this API would still need to check for the existency of a certain key in the returned hash.
+ } + else { + if (add_string (&ret, p) == -1) { + return NULL; + } + if (add_string (&ret, "") == -1) { + return NULL; + }
As above, I would not add elements with empty values in the hash.
+ } + + p = pend; + } + + if (end_stringsbuf (&ret) == -1) + return NULL; + + return ret.argv; + +} diff --git a/generator/actions.ml b/generator/actions.ml index cf96039..24d3ecc 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -12014,6 +12014,15 @@ This is the internal call which implements C<guestfs_lstatnslist>." }; longdesc = "\ Get the default subvolume or snapshot of a filesystem mounted at C<mountpoint>." };
+ { defaults with + name = "btrfs_subvolume_show"; + style = RHashtable "btrfssubvolumeinfo", [Pathname "subvolume"], []; + proc_nr = Some 425; + optional = Some "btrfs"; camel_name = "BTRFSSubvolumeShow"; + shortdesc = "show detailed information of the subvolume";
"returns detailed information about the subvolume"
+ longdesc = "\ +show detailed information of the subvolume." };
Like above, making sure it is properly capitalized at the beginning. -- Pino Toscano
On Fri, Nov 21, 2014 at 05:17:13PM +0100, Pino Toscano wrote:
On Friday 21 November 2014 13:18:00 Hu Tao wrote:
btrfs_subvolume_show shows the detailed information of a subvolume or snapshot.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- daemon/btrfs.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++++++ generator/actions.ml | 9 +++ src/MAX_PROC_NR | 2 +- 3 files changed, 177 insertions(+), 1 deletion(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c index e179b57..36ba588 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -29,6 +29,7 @@ #include "actions.h" #include "optgroups.h" #include "xstrtol.h" +#include "c-ctype.h"
GUESTFSD_EXT_CMD(str_btrfs, btrfs); GUESTFSD_EXT_CMD(str_btrfstune, btrfstune); @@ -813,3 +814,169 @@ do_btrfs_fsck (const char *device, int64_t superblock, int repair)
return 0; } + +char **do_btrfs_subvolume_show (const char *subvolume) +{ + const size_t MAX_ARGS = 64; + const char *argv[MAX_ARGS]; + size_t i = 0; + CLEANUP_FREE char *subvolume_buf = NULL; + CLEANUP_FREE char *err = NULL; + CLEANUP_FREE char *out = NULL; + char *p, *pend, *colon; + DECLARE_STRINGSBUF (ret); + int r; + + subvolume_buf = sysroot_path (subvolume); + if (subvolume_buf == NULL) { + reply_with_perror ("malloc"); + return NULL; + } + + ADD_ARG (argv, i, str_btrfs); + ADD_ARG (argv, i, "subvolume"); + ADD_ARG (argv, i, "show"); + ADD_ARG (argv, i, subvolume_buf); + ADD_ARG (argv, i, NULL); + + r = commandv (&out, &err, argv); + if (r == -1) { + reply_with_error ("%s: %s", subvolume, err); + return NULL; + } + + /* Output is: + * + * / + * Name: root + * uuid: c875169e-cf4e-a04d-9959-b667dec36234 + * Parent uuid: - + * Creation time: 2014-11-13 10:13:08 + * Object ID: 256 + * Generation (Gen): 6579 + * Gen at creation: 5 + * Parent: 5 + * Top Level: 5 + * Flags: - + * Snapshot(s): + * snapshots/test1 + * snapshots/test2 + * snapshots/test3 + * + */ + + p = out; + + p = strchr (p, '\n'); + if (p) { + *p = '\0'; + p++; + } + else {
Minor style note: } else {
(also in other parts of this patch)
Though I'm fine with it, but do we have coding style rules? I see both styles in existing codes.
+ reply_with_error ("truncated output");
I'd print out anyway, to ease debugging failures a bit.
Okay.
+ return NULL; + } + + /* If the path is the btrfs root, `btrfs subvolume show' reports: + * <path> is btrfs root + */ + if (strstr(out, "is btrfs root") != NULL) {
Take care of adding a space before parenthesis (also in other parts of this patch).
Okay.
+ reply_with_error ("%s is btrfs root", subvolume); + return NULL; + } + + /* The first line is the path of the subvolume. */ + if (add_string (&ret, strdup("path")) == -1) {
add_string duplicates the string, so you don't need to do that manually.
Okay.
+ return NULL; + } + if (add_string (&ret, out) == -1) { + return NULL; + } + + /* Read the lines and split into "key: value". */ + while (*p) { + /* leading spaces and tabs */ + while (*p && c_isspace (*p)) p++;
Properly indent such lines, so: while (*p && c_isspace (*p)) p++;
(also, a minor optimization is to use ++p instead of p++, as it can avoid a temporary value)
Okay.
+ + pend = strchrnul (p, '\n'); + if (*pend == '\n') { + *pend = '\0'; + pend++; + } + + if (!*p) continue; + + colon = strchr (p, ':'); + if (colon) { + *colon = '\0'; + + /* snapshot is special, see the output above */ + if (strncmp(p, "Snapshot", sizeof("Snapshot") - 1) == 0) {
Please use the STR* macros we have in src/guestfs-internal-all.h.
Also, most probably it is better to match "Snapshot(s)", so if in the future a new field starting with "Snapshot" is added then it won't be mistaken for this multiline one.
Sounds good.
+ char *ss = NULL; + + if (add_string (&ret, p) == -1) { + return NULL; + }
Single statements don't need curly brackets (also in other parts of the patch).
I think it is better to document this one, too.
+ + /* each line is the path of a snapshot. */ + p = pend; + while (*p && strchr(p, ':') == NULL) {
This will break once the output will have fields after Snapshot, as strchr(p, ':') will return a non-null value in one of the lines following the first line after the Snapshot one.
It is intended, so that we can parse properly possibly key:value pairs following "Snapshot(s)". Although a ':' in a snapshot path will hit it too, but people don't likely put a ':' in path.
+ while (*p && c_isspace (*p)) p++; + pend = strchrnul (p, '\n'); + if (*pend == '\n') { + *pend = '\0'; + pend++; + }
I see this code repeated already; I think it would be better to create a small helper function like: char *analyze_line (char *p, char **field, char **value) which would - destructively update p (setting null bytes, just like it is done now) - return in field the start of the field text - return in value, if present, the start of the value (after the colon) - return the pointer to the first character of the new line (or null, if none following) this way it is IMHO easier to iterate over the lines, and also to detect whether a line contains a field+value or just text.
How to treat lines don't containing a colon? treat them as invalid line? or as value of key in previous line? Note this should be a general process.
+ if (ss) { + ss = realloc(ss, strlen(ss) + strlen(p));
If realloc fails, the return value is null; this means that the following will leak ss in that case.
Also, sounds like you are missing "," in the length of the string?
+ strcat(ss, ",");
Just save the length of ss before the realloc, and you can simply do ss[old_length_of_ss] = ',';
+ strcat(ss, p);
You can easily use memcpy here: memcpy (ss + old_length_of_ss + 1, p, strlen (p));
+ } else { + ss = strdup(p);
Missing check for failed strdup.
+ } + + p = pend; + } + + if (ss) { + if (add_string (&ret, ss) == -1) {
This needs to be add_string_nodup.
+ free(ss);
No need to free ss here; theoretically add_string will take care of it, be it added to the stringbuf or freed on error.
This is fail case.
+ return NULL; + } + } + else { + if (add_string (&ret, strdup("")) == -1) { + return NULL; + } + }
If there are no elements for "Snapshot(s)", then it would be better to just not add an empty hash element.
+ + continue; + } + + do { colon++; } while (*colon && c_isspace (*colon)); + if (add_string (&ret, p) == -1) { + return NULL; + } + if (add_string (&ret, colon) == -1) { + return NULL; + }
At least from the example output added as comment, it seems that "not available" values might be "-" -- it'd just not add those, since as users of this API would still need to check for the existency of a certain key in the returned hash.
tune2fs-l lists keys with empty values, shouldn't we follow it?
+ } + else { + if (add_string (&ret, p) == -1) { + return NULL; + } + if (add_string (&ret, "") == -1) { + return NULL; + }
As above, I would not add elements with empty values in the hash.
+ } + + p = pend; + } + + if (end_stringsbuf (&ret) == -1) + return NULL; + + return ret.argv; + +} diff --git a/generator/actions.ml b/generator/actions.ml index cf96039..24d3ecc 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -12014,6 +12014,15 @@ This is the internal call which implements C<guestfs_lstatnslist>." }; longdesc = "\ Get the default subvolume or snapshot of a filesystem mounted at C<mountpoint>." };
+ { defaults with + name = "btrfs_subvolume_show"; + style = RHashtable "btrfssubvolumeinfo", [Pathname "subvolume"], []; + proc_nr = Some 425; + optional = Some "btrfs"; camel_name = "BTRFSSubvolumeShow"; + shortdesc = "show detailed information of the subvolume";
"returns detailed information about the subvolume"
+ longdesc = "\ +show detailed information of the subvolume." };
Like above, making sure it is properly capitalized at the beginning.
sure. Thank you for reviewing!
-- Pino Toscano
On Monday 24 November 2014 16:22:53 Hu Tao wrote:
On Fri, Nov 21, 2014 at 05:17:13PM +0100, Pino Toscano wrote:
On Friday 21 November 2014 13:18:00 Hu Tao wrote:
btrfs_subvolume_show shows the detailed information of a subvolume or snapshot.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> ---
daemon/btrfs.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++++++ generator/actions.ml | 9 +++ src/MAX_PROC_NR | 2 +- 3 files changed, 177 insertions(+), 1 deletion(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c index e179b57..36ba588 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -29,6 +29,7 @@
#include "actions.h" #include "optgroups.h" #include "xstrtol.h"
+#include "c-ctype.h"
GUESTFSD_EXT_CMD(str_btrfs, btrfs); GUESTFSD_EXT_CMD(str_btrfstune, btrfstune);
@@ -813,3 +814,169 @@ do_btrfs_fsck (const char *device, int64_t superblock, int repair)> > return 0;
}
+ +char **do_btrfs_subvolume_show (const char *subvolume) +{ + const size_t MAX_ARGS = 64; + const char *argv[MAX_ARGS]; + size_t i = 0; + CLEANUP_FREE char *subvolume_buf = NULL; + CLEANUP_FREE char *err = NULL; + CLEANUP_FREE char *out = NULL; + char *p, *pend, *colon; + DECLARE_STRINGSBUF (ret); + int r; + + subvolume_buf = sysroot_path (subvolume); + if (subvolume_buf == NULL) { + reply_with_perror ("malloc"); + return NULL; + } + + ADD_ARG (argv, i, str_btrfs); + ADD_ARG (argv, i, "subvolume"); + ADD_ARG (argv, i, "show"); + ADD_ARG (argv, i, subvolume_buf); + ADD_ARG (argv, i, NULL); + + r = commandv (&out, &err, argv); + if (r == -1) { + reply_with_error ("%s: %s", subvolume, err); + return NULL; + } + + /* Output is: + * + * / + * Name: root + * uuid: c875169e-cf4e-a04d-9959-b667dec36234 + * Parent uuid: - + * Creation time: 2014-11-13 10:13:08 + * Object ID: 256 + * Generation (Gen): 6579 + * Gen at creation: 5 + * Parent: 5 + * Top Level: 5 + * Flags: - + * Snapshot(s): + * snapshots/test1 + * snapshots/test2 + * snapshots/test3 + * + */ + + p = out; + + p = strchr (p, '\n'); + if (p) { + *p = '\0'; + p++; + } + else {
Minor style note: } else {
(also in other parts of this patch)
Though I'm fine with it, but do we have coding style rules? I see both styles in existing codes.
From what I see, «} else {» is more common.
+ + /* each line is the path of a snapshot. */ + p = pend; + while (*p && strchr(p, ':') == NULL) {
This will break once the output will have fields after Snapshot, as strchr(p, ':') will return a non-null value in one of the lines following the first line after the Snapshot one.
It is intended, so that we can parse properly possibly key:value pairs following "Snapshot(s)". Although a ':' in a snapshot path will hit it too, but people don't likely put a ':' in path.
Well, that's not what would happen: "p" points to the first character of the line after the "Snapshot(s)" one, and strchr() will try to find ':' until the end of the string, not stopping at newlines and such. Let's say the output is something like: Name: root uuid: c875169e-cf4e-a04d-9959-b667dec36234 Parent uuid: - Creation time: 2014-11-13 10:13:08 Object ID: 256 Generation (Gen): 6579 Gen at creation: 5 Parent: 5 Top Level: 5 Flags: - Snapshot(s): snapshots/test1 snapshots/test2 snapshots/test3 Other attribute: - p would be at: snapshots/test1 ^- here, while strchr(p, ':') == NULL) would be Other attribute: - ^- here, so skipping all the multi-line value.
+ while (*p && c_isspace (*p)) p++; + pend = strchrnul (p, '\n'); + if (*pend == '\n') { + *pend = '\0'; + pend++; + }
I see this code repeated already; I think it would be better to create> a small helper function like: char *analyze_line (char *p, char **field, char **value)
which would - destructively update p (setting null bytes, just like it is done now) - return in field the start of the field text - return in value, if present, the start of the value (after the colon) - return the pointer to the first character of the new line (or null, if none following)
this way it is IMHO easier to iterate over the lines, and also to detect whether a line contains a field+value or just text.
How to treat lines don't containing a colon? treat them as invalid line? or as value of key in previous line? Note this should be a general process.
field pointing at their non-blank start, and value as null.
+ free(ss);
No need to free ss here; theoretically add_string will take care of it, be it added to the stringbuf or freed on error.
This is fail case.
I don't understand what you mean here, can you please be more verbose?
+ + continue; + } + + do { colon++; } while (*colon && c_isspace (*colon)); + if (add_string (&ret, p) == -1) { + return NULL; + } + if (add_string (&ret, colon) == -1) { + return NULL; + }
At least from the example output added as comment, it seems that "not available" values might be "-" -- it'd just not add those, since as users of this API would still need to check for the existency of a certain key in the returned hash.
tune2fs-l lists keys with empty values, shouldn't we follow it?
Do you have an example of them? I haven't used tune2fs-l much myself, but I don't remember having seen empty values in its output. -- Pino Toscano
On Mon, Nov 24, 2014 at 06:17:22PM +0100, Pino Toscano wrote:
On Monday 24 November 2014 16:22:53 Hu Tao wrote:
On Fri, Nov 21, 2014 at 05:17:13PM +0100, Pino Toscano wrote:
On Friday 21 November 2014 13:18:00 Hu Tao wrote:
btrfs_subvolume_show shows the detailed information of a subvolume or snapshot.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> ---
daemon/btrfs.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++++++ generator/actions.ml | 9 +++ src/MAX_PROC_NR | 2 +- 3 files changed, 177 insertions(+), 1 deletion(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c index e179b57..36ba588 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -29,6 +29,7 @@
#include "actions.h" #include "optgroups.h" #include "xstrtol.h"
+#include "c-ctype.h"
GUESTFSD_EXT_CMD(str_btrfs, btrfs); GUESTFSD_EXT_CMD(str_btrfstune, btrfstune);
@@ -813,3 +814,169 @@ do_btrfs_fsck (const char *device, int64_t superblock, int repair)> > return 0;
}
+ +char **do_btrfs_subvolume_show (const char *subvolume) +{ + const size_t MAX_ARGS = 64; + const char *argv[MAX_ARGS]; + size_t i = 0; + CLEANUP_FREE char *subvolume_buf = NULL; + CLEANUP_FREE char *err = NULL; + CLEANUP_FREE char *out = NULL; + char *p, *pend, *colon; + DECLARE_STRINGSBUF (ret); + int r; + + subvolume_buf = sysroot_path (subvolume); + if (subvolume_buf == NULL) { + reply_with_perror ("malloc"); + return NULL; + } + + ADD_ARG (argv, i, str_btrfs); + ADD_ARG (argv, i, "subvolume"); + ADD_ARG (argv, i, "show"); + ADD_ARG (argv, i, subvolume_buf); + ADD_ARG (argv, i, NULL); + + r = commandv (&out, &err, argv); + if (r == -1) { + reply_with_error ("%s: %s", subvolume, err); + return NULL; + } + + /* Output is: + * + * / + * Name: root + * uuid: c875169e-cf4e-a04d-9959-b667dec36234 + * Parent uuid: - + * Creation time: 2014-11-13 10:13:08 + * Object ID: 256 + * Generation (Gen): 6579 + * Gen at creation: 5 + * Parent: 5 + * Top Level: 5 + * Flags: - + * Snapshot(s): + * snapshots/test1 + * snapshots/test2 + * snapshots/test3 + * + */ + + p = out; + + p = strchr (p, '\n'); + if (p) { + *p = '\0'; + p++; + } + else {
Minor style note: } else {
(also in other parts of this patch)
Though I'm fine with it, but do we have coding style rules? I see both styles in existing codes.
From what I see, «} else {» is more common.
Okay.
+ + /* each line is the path of a snapshot. */ + p = pend; + while (*p && strchr(p, ':') == NULL) {
This will break once the output will have fields after Snapshot, as strchr(p, ':') will return a non-null value in one of the lines following the first line after the Snapshot one.
It is intended, so that we can parse properly possibly key:value pairs following "Snapshot(s)". Although a ':' in a snapshot path will hit it too, but people don't likely put a ':' in path.
Well, that's not what would happen: "p" points to the first character of the line after the "Snapshot(s)" one, and strchr() will try to find ':' until the end of the string, not stopping at newlines and such.
Let's say the output is something like:
Name: root uuid: c875169e-cf4e-a04d-9959-b667dec36234 Parent uuid: - Creation time: 2014-11-13 10:13:08 Object ID: 256 Generation (Gen): 6579 Gen at creation: 5 Parent: 5 Top Level: 5 Flags: - Snapshot(s): snapshots/test1 snapshots/test2 snapshots/test3 Other attribute: -
p would be at: snapshots/test1 ^- here, while strchr(p, ':') == NULL) would be Other attribute: - ^- here, so skipping all the multi-line value.
You're right:) fixed.
+ while (*p && c_isspace (*p)) p++; + pend = strchrnul (p, '\n'); + if (*pend == '\n') { + *pend = '\0'; + pend++; + }
I see this code repeated already; I think it would be better to create> a small helper function like: char *analyze_line (char *p, char **field, char **value)
which would - destructively update p (setting null bytes, just like it is done now) - return in field the start of the field text - return in value, if present, the start of the value (after the colon) - return the pointer to the first character of the new line (or null, if none following)
this way it is IMHO easier to iterate over the lines, and also to detect whether a line contains a field+value or just text.
How to treat lines don't containing a colon? treat them as invalid line? or as value of key in previous line? Note this should be a general process.
field pointing at their non-blank start, and value as null.
I am now realizing I asked a question not related to analyze_line(). (I am trying to elaborate it below but not rasing a question) Say we have these lines: key1: value1 value2 value3 key2 <--- (this is a malformed string lacks : and value) for each lines, we will get following results: field: key1 value: "" field: value1 value: (null) field: value2 value: (null) field: value3 value: (null) field: key2 value: (null) for "value1"-"value3", we can append them to value of key1, because this is what we want. But for "key2", we append it as value too. In another word, we don't have the ability to detect malformed lines. However, this is what I've done in my patch.
+ free(ss);
No need to free ss here; theoretically add_string will take care of it, be it added to the stringbuf or freed on error.
This is fail case.
I don't understand what you mean here, can you please be more verbose?
The code is: if (add_string (&ret, ss) == -1) { free (ss); return NULL; } So the ss is not yet added to stringbug, I think in the case ss should be freed manually. But if you mean ss will be freed later even in this case (I'm not sure about it), then we can remove the free line safely.
+ + continue; + } + + do { colon++; } while (*colon && c_isspace (*colon)); + if (add_string (&ret, p) == -1) { + return NULL; + } + if (add_string (&ret, colon) == -1) { + return NULL; + }
At least from the example output added as comment, it seems that "not available" values might be "-" -- it'd just not add those, since as users of this API would still need to check for the existency of a certain key in the returned hash.
tune2fs-l lists keys with empty values, shouldn't we follow it?
Do you have an example of them? I haven't used tune2fs-l much myself, but I don't remember having seen empty values in its output.
See below, tune2fs-l on a freshly mkfs-ed ext3 filesystem shows some blank fields:
<fs> mkfs ext3 /dev/sda6 <fs> tune2fs-l /dev/sda6 Filesystem volume name: Last mounted on: Filesystem UUID: e4ee511c-c8c6-4e6a-baee-9b7e1375b296 Filesystem magic number: 0xEF53 Filesystem revision #: 1 (dynamic) Filesystem features: ext_attr resize_inode dir_index filetype sparse_super Filesystem flags: signed_directory_hash Default mount options: user_xattr acl Filesystem state: clean Errors behavior: Continue Filesystem OS type: Linux Inode count: 128 Block count: 1020 Reserved block count: 51 Free blocks: 982 Free inodes: 117 First block: 1 Block size: 1024 Fragment size: 1024 Reserved GDT blocks: 3 Blocks per group: 8192 Fragments per group: 8192 Inodes per group: 128 Inode blocks per group: 16 Filesystem created: Wed Nov 26 02:25:19 2014 Last mount time: n/a Last write time: Wed Nov 26 02:25:19 2014 Mount count: 0 Maximum mount count: -1 Last checked: Wed Nov 26 02:25:19 2014 Check interval: 0 (<none>) Reserved blocks uid: 0 (user root) Reserved blocks gid: 0 (group root) First inode: 11 Inode size: 128 Default directory hash: half_md4 Directory Hash Seed: dd7fbb80-1356-47c8-b99e-cbec6970df15
-- Pino Toscano
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
On Fri, Nov 21, 2014 at 05:17:13PM +0100, Pino Toscano wrote: <...>
+ while (*p && c_isspace (*p)) p++; + pend = strchrnul (p, '\n'); + if (*pend == '\n') { + *pend = '\0'; + pend++; + }
I see this code repeated already; I think it would be better to create a small helper function like: char *analyze_line (char *p, char **field, char **value)
perhaps an extra delimiter parameter?
which would - destructively update p (setting null bytes, just like it is done now) - return in field the start of the field text - return in value, if present, the start of the value (after the colon) - return the pointer to the first character of the new line (or null, if none following) this way it is IMHO easier to iterate over the lines, and also to detect whether a line contains a field+value or just text.
Regards, Hu
participants (2)
-
Hu Tao -
Pino Toscano