[PATCH 0/2] New API: part_get_part_type
Chen Hanxiao (2): parted: introduce enum for whether parted has option -m New API: part_get_part_type for showing partition type daemon/parted.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++----- generator/actions.ml | 18 +++++++ src/MAX_PROC_NR | 2 +- 3 files changed, 143 insertions(+), 13 deletions(-) -- 2.1.0
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- daemon/parted.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/daemon/parted.c b/daemon/parted.c index a7bcb99..64a7d3c 100644 --- a/daemon/parted.c +++ b/daemon/parted.c @@ -33,6 +33,12 @@ GUESTFSD_EXT_CMD(str_parted, parted); GUESTFSD_EXT_CMD(str_sfdisk, sfdisk); GUESTFSD_EXT_CMD(str_sgdisk, sgdisk); +enum { + PARTED_INVALID = -1, + /* parted do not support -m option */ + PARTED_OPT_NO_M, + PARTED_OPT_HAS_M}; + /* Notes: * * Parted 1.9 sends error messages to stdout, hence use of the @@ -320,7 +326,7 @@ get_table_field (const char *line, int n) static int test_parted_m_opt (void) { - static int result = -1; + static int result = PARTED_INVALID; if (result >= 0) return result; @@ -334,9 +340,9 @@ test_parted_m_opt (void) } if (err && strstr (err, "invalid option -- m")) - result = 0; + result = PARTED_OPT_NO_M; else - result = 1; + result = PARTED_OPT_HAS_M; return result; } @@ -347,7 +353,7 @@ print_partition_table (const char *device, int parted_has_m_opt) CLEANUP_FREE char *err = NULL; int r; - if (parted_has_m_opt) + if (PARTED_OPT_HAS_M == parted_has_m_opt) r = command (&out, &err, str_parted, "-m", "--", device, "unit", "b", "print", NULL); @@ -370,14 +376,14 @@ char * do_part_get_parttype (const char *device) { int parted_has_m_opt = test_parted_m_opt (); - if (parted_has_m_opt == -1) + if (parted_has_m_opt == PARTED_INVALID) return NULL; CLEANUP_FREE char *out = print_partition_table (device, parted_has_m_opt); if (!out) return NULL; - if (parted_has_m_opt) { + if (PARTED_OPT_HAS_M == parted_has_m_opt) { /* New-style parsing using the "machine-readable" format from * 'parted -m'. */ @@ -452,7 +458,7 @@ guestfs_int_partition_list * do_part_list (const char *device) { int parted_has_m_opt = test_parted_m_opt (); - if (parted_has_m_opt == -1) + if (parted_has_m_opt == PARTED_INVALID) return NULL; CLEANUP_FREE char *out = print_partition_table (device, parted_has_m_opt); @@ -466,7 +472,7 @@ do_part_list (const char *device) guestfs_int_partition_list *r; - if (parted_has_m_opt) { + if (PARTED_OPT_HAS_M == parted_has_m_opt) { /* New-style parsing using the "machine-readable" format from * 'parted -m'. * @@ -578,7 +584,7 @@ do_part_get_bootable (const char *device, int partnum) } int parted_has_m_opt = test_parted_m_opt (); - if (parted_has_m_opt == -1) + if (parted_has_m_opt == PARTED_INVALID) return -1; CLEANUP_FREE char *out = print_partition_table (device, parted_has_m_opt); @@ -590,7 +596,7 @@ do_part_get_bootable (const char *device, int partnum) if (!lines) return -1; - if (parted_has_m_opt) { + if (PARTED_OPT_HAS_M == parted_has_m_opt) { /* New-style parsing using the "machine-readable" format from * 'parted -m'. * @@ -965,14 +971,14 @@ do_part_get_name (const char *device, int partnum) if (STREQ (parttype, "gpt")) { int parted_has_m_opt = test_parted_m_opt (); - if (parted_has_m_opt == -1) + if (parted_has_m_opt == PARTED_INVALID) return NULL; CLEANUP_FREE char *out = print_partition_table (device, parted_has_m_opt); if (!out) return NULL; - if (parted_has_m_opt) { + if (PARTED_OPT_HAS_M == parted_has_m_opt) { /* New-style parsing using the "machine-readable" format from * 'parted -m'. */ -- 2.1.0
On Tuesday 24 March 2015 07:20:16 Chen Hanxiao wrote:
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- daemon/parted.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/daemon/parted.c b/daemon/parted.c index a7bcb99..64a7d3c 100644 --- a/daemon/parted.c +++ b/daemon/parted.c @@ -33,6 +33,12 @@ GUESTFSD_EXT_CMD(str_parted, parted); GUESTFSD_EXT_CMD(str_sfdisk, sfdisk); GUESTFSD_EXT_CMD(str_sgdisk, sgdisk);
+enum { + PARTED_INVALID = -1, + /* parted do not support -m option */ + PARTED_OPT_NO_M, + PARTED_OPT_HAS_M};
(I didn't have even the time to reply to the question about this enum) PARTED_INVALID does not make much sense, especially that I was referring just to the parameter for print_partition_table, so for it only two values (eg PARTED_OUTPUT_MACHINE and PARTED_OUTPUT_NORMAL) are enough.
+ /* Notes: * * Parted 1.9 sends error messages to stdout, hence use of the @@ -320,7 +326,7 @@ get_table_field (const char *line, int n) static int test_parted_m_opt (void) { - static int result = -1; + static int result = PARTED_INVALID;
Commenting here, but it applies to the rest of the changes: if you want to apply an enum for this case, then do it consistently and for all the cases. Using an enum value and storing it to an int variable defeats the point of using an enum at all, as you will not catch non-enum values or not check to be handling all values where needed. -- Pino Toscano
On Tue, Mar 24, 2015 at 01:15:21PM +0100, Pino Toscano wrote:
On Tuesday 24 March 2015 07:20:16 Chen Hanxiao wrote:
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- daemon/parted.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/daemon/parted.c b/daemon/parted.c index a7bcb99..64a7d3c 100644 --- a/daemon/parted.c +++ b/daemon/parted.c @@ -33,6 +33,12 @@ GUESTFSD_EXT_CMD(str_parted, parted); GUESTFSD_EXT_CMD(str_sfdisk, sfdisk); GUESTFSD_EXT_CMD(str_sgdisk, sgdisk);
+enum { + PARTED_INVALID = -1, + /* parted do not support -m option */ + PARTED_OPT_NO_M, + PARTED_OPT_HAS_M};
(I didn't have even the time to reply to the question about this enum)
PARTED_INVALID does not make much sense, especially that I was referring just to the parameter for print_partition_table, so for it only two values (eg PARTED_OUTPUT_MACHINE and PARTED_OUTPUT_NORMAL) are enough.
+ /* Notes: * * Parted 1.9 sends error messages to stdout, hence use of the @@ -320,7 +326,7 @@ get_table_field (const char *line, int n) static int test_parted_m_opt (void) { - static int result = -1; + static int result = PARTED_INVALID;
Commenting here, but it applies to the rest of the changes: if you want to apply an enum for this case, then do it consistently and for all the cases. Using an enum value and storing it to an int variable defeats the point of using an enum at all, as you will not catch non-enum values or not check to be handling all values where needed.
Agreed. However I'm just going to make this fix and push it. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
-----Original Message----- From: libguestfs-bounces@redhat.com [mailto:libguestfs-bounces@redhat.com] On Behalf Of Richard W.M. Jones Sent: Tuesday, March 24, 2015 9:31 PM To: Pino Toscano Cc: libguestfs@redhat.com Subject: Re: [Libguestfs] [PATCH 1/2] parted: introduce enum for whether parted has option -m
On Tue, Mar 24, 2015 at 01:15:21PM +0100, Pino Toscano wrote:
On Tuesday 24 March 2015 07:20:16 Chen Hanxiao wrote:
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- daemon/parted.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/daemon/parted.c b/daemon/parted.c index a7bcb99..64a7d3c 100644 --- a/daemon/parted.c +++ b/daemon/parted.c @@ -33,6 +33,12 @@ GUESTFSD_EXT_CMD(str_parted, parted); GUESTFSD_EXT_CMD(str_sfdisk, sfdisk); GUESTFSD_EXT_CMD(str_sgdisk, sgdisk);
+enum { + PARTED_INVALID = -1, + /* parted do not support -m option */ + PARTED_OPT_NO_M, + PARTED_OPT_HAS_M};
(I didn't have even the time to reply to the question about this enum)
PARTED_INVALID does not make much sense, especially that I was referring just to the parameter for print_partition_table, so for it only two values (eg PARTED_OUTPUT_MACHINE and PARTED_OUTPUT_NORMAL) are enough.
+ /* Notes: * * Parted 1.9 sends error messages to stdout, hence use of the @@ -320,7 +326,7 @@ get_table_field (const char *line, int n) static int test_parted_m_opt (void) { - static int result = -1; + static int result = PARTED_INVALID;
Commenting here, but it applies to the rest of the changes: if you want to apply an enum for this case, then do it consistently and for all the cases. Using an enum value and storing it to an int variable defeats the point of using an enum at all, as you will not catch non-enum values or not check to be handling all values where needed.
Agreed.
However I'm just going to make this fix and push it.
Hi Rich, Pino Thanks for your help. Regards, - Chen
This patch will add support for getting partition type of a partiton numbered device. Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- daemon/parted.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++ generator/actions.ml | 18 +++++++++ src/MAX_PROC_NR | 2 +- 3 files changed, 125 insertions(+), 1 deletion(-) diff --git a/daemon/parted.c b/daemon/parted.c index 64a7d3c..3aac102 100644 --- a/daemon/parted.c +++ b/daemon/parted.c @@ -1028,3 +1028,109 @@ do_part_get_name (const char *device, int partnum) reply_with_error ("cannot get the partition name from '%s' layouts", parttype); return NULL; } + +char * +do_part_get_part_type (const char *device, int partnum) +{ + CLEANUP_FREE char *parttype; + char *part_type; + + parttype = do_part_get_parttype (device); + if (parttype == NULL) + return NULL; + + /* machine parseable output by 'parted -m' did not provide + * partition type info. + * Use traditional style. + */ + CLEANUP_FREE char *out = print_partition_table (device, PARTED_OPT_NO_M); + if (!out) + return NULL; + + CLEANUP_FREE_STRING_LIST char **lines = split_lines (out); + + if (!lines) + return NULL; + + size_t start = 0, end = 0, row; + + for (row = 0; lines[row] != NULL; ++row) + if (STRPREFIX (lines[row], "Number")) { + start = row + 1; + break; + } + + if (start == 0) { + reply_with_error ("parted output has no \"Number\" line"); + return NULL; + } + + for (row = start; lines[row] != NULL; ++row) + if (STREQ (lines[row], "")) { + end = row; + break; + } + + if (end == 0) { + reply_with_error ("parted output has no blank after end of table"); + return NULL; + } + + /* Now parse the lines. */ + size_t i; + int64_t temp_int64; + int part_num; + char temp_type[16] = {'\0'}; + for (i = 0, row = start; row < end; ++i, ++row) { + if (STREQ (parttype, "gpt")) { + memcpy (temp_type, "primary", strlen("primary")); + if (sscanf (lines[row], "%d%" SCNi64 "B%" SCNi64 "B%" SCNi64 "B", + &part_num, + &temp_int64, + &temp_int64, + &temp_int64) != 4) { + reply_with_error ("could not parse row from output of parted print command: %s", lines[row]); + return NULL; + } + } else { + if (sscanf (lines[row], "%d%" SCNi64 "B%" SCNi64 "B%" SCNi64 "B" "%15s", + &part_num, + &temp_int64, + &temp_int64, + &temp_int64, + temp_type) != 5) { + reply_with_error ("could not parse row from output of parted print command: %s", lines[row]); + return NULL; + } + } + + if (part_num != partnum) + continue; + + if (STRPREFIX (temp_type, "primary")) { + part_type = strdup("primary"); + if (part_type == NULL) + goto error; + } else if (STRPREFIX (temp_type, "logical")) { + part_type = strdup("logical"); + if (part_type == NULL) + goto error; + } else if (STRPREFIX (temp_type, "extended")) { + part_type = strdup("extended"); + if (part_type == NULL) + goto error; + } else + goto error; + + return part_type; + } + + if (row == end) { + reply_with_error ("could not find partnum: %d", partnum); + return NULL; + } + + error: + reply_with_error ("strdup failed"); + return NULL; +} diff --git a/generator/actions.ml b/generator/actions.ml index 786b9a2..ff8fd87 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -12527,6 +12527,24 @@ This will Enable extended inode refs." }; longdesc = "\ This enable skinny metadata extent refs." }; + { defaults with + name = "part_get_part_type"; + style = RString "partitiontype", [Device "device"; Int "partnum"], []; + proc_nr = Some 453; + tests = [ + InitEmpty, Always, TestResultString ( + [["part_init"; "/dev/sda"; "mbr"]; + ["part_add"; "/dev/sda"; "p"; "64"; "204799"]; + ["part_add"; "/dev/sda"; "e"; "204800"; "614400"]; + ["part_add"; "/dev/sda"; "l"; "204864"; "205988"]; + ["part_get_part_type"; "/dev/sda"; "5"]], "logical"), [] + ]; + + shortdesc = "get the partition type"; + longdesc = "\ +This could get the partition type, such as primary, logical, +on partition numbered C<partnum> on device C<device>." }; + ] (* Non-API meta-commands available only in guestfish. diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR index 8670c73..534b992 100644 --- a/src/MAX_PROC_NR +++ b/src/MAX_PROC_NR @@ -1 +1 @@ -452 +453 -- 2.1.0
On Tue, Mar 24, 2015 at 07:20:17AM -0400, Chen Hanxiao wrote:
This patch will add support for getting partition type of a partiton numbered device.
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
I have pushed this. I changed the API name to part_get_mbr_part_type since it's really MBR-specific, and also we have an API called part_get_parttype[1] so there was an opportunity for confusion. Also I added an extra test and made a small adjustment to the API description. Here are the upstream commits: https://github.com/libguestfs/libguestfs/commit/af9aa2eb0582bfced4e16ade73c1... https://github.com/libguestfs/libguestfs/commit/0c396a4bce578486dfc4a38e1f8c... Thanks, Rich. [1] returns the partition table type like "mbr". -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
-----Original Message----- From: Richard W.M. Jones [mailto:rjones@redhat.com] Sent: Tuesday, March 24, 2015 9:52 PM To: Chen, Hanxiao/陈 晗霄 Cc: libguestfs@redhat.com Subject: Re: [Libguestfs] [PATCH 2/2] New API: part_get_part_type for showing partition type
On Tue, Mar 24, 2015 at 07:20:17AM -0400, Chen Hanxiao wrote:
This patch will add support for getting partition type of a partiton numbered device.
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
I have pushed this.
I changed the API name to part_get_mbr_part_type since it's really MBR-specific, and also we have an API called part_get_parttype[1] so there was an opportunity for confusion.
Also I added an extra test and made a small adjustment to the API description.
Here are the upstream commits:
https://github.com/libguestfs/libguestfs/commit/af9aa2eb0582bfced4e16ade73c1... 321b91ceb9 https://github.com/libguestfs/libguestfs/commit/0c396a4bce578486dfc4a38e1f8c... fd5c2836ea
Thanks,
Rich.
[1] returns the partition table type like "mbr".
Thanks for your help and links. Regards, - Chen
participants (4)
-
Chen Hanxiao -
Chen, Hanxiao
-
Pino Toscano -
Richard W.M. Jones