Re: [Qemu-block] [PATCH v3 27/36] sheepdog: QAPIfy "redundancy" create option

[Max Reitz]

On 2018-02-23 20:25, Kevin Wolf wrote:
> The "redundancy" option for Sheepdog image creation is currently a
> string that can encode one or two integers depending on its format,
> which at the same time implicitly selects a mode.
> 
> This patch turns it into a QAPI union and converts the string into such
> a QAPI object before interpreting the values.
> 
> Signed-off-by: Kevin Wolf 
> Reviewed-by: Max Reitz 
> ---
>  qapi/block-core.json | 45 +
>  block/sheepdog.c | 94 
> +---
>  2 files changed, 112 insertions(+), 27 deletions(-)

[...]

> @@ -1907,35 +1950,32 @@ static int parse_redundancy(BDRVSheepdogState *s, 
> const char *opt)
>  return -EINVAL;
>  }
>  
> -copy = strtol(n1, NULL, 10);
> -/* FIXME fix error checking by switching to qemu_strtol() */
> -if (copy > SD_MAX_COPIES || copy < 1) {
> -return -EINVAL;
> -}
> -if (!n2) {
> -inode->copy_policy = 0;
> -inode->nr_copies = copy;
> -return 0;
> +ret = qemu_strtol(n1, NULL, 10, );

(By the way: This was what I was thanking you for in v2 -- I just now
realized I was clever enough not to point to it in my reply...)

> +if (ret < 0) {
> +return ret;
>  }



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v3 27/36] sheepdog: QAPIfy "redundancy" create option

[Kevin Wolf]

The "redundancy" option for Sheepdog image creation is currently a
string that can encode one or two integers depending on its format,
which at the same time implicitly selects a mode.

This patch turns it into a QAPI union and converts the string into such
a QAPI object before interpreting the values.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 qapi/block-core.json | 45 +
 block/sheepdog.c | 94 +---
 2 files changed, 112 insertions(+), 27 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 085b791303..2b249c9e3d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3467,6 +3467,51 @@
 '*cluster-size' :   'size' } }
 
 ##
+# @SheepdogRedundancyType:
+#
+# @full Create a fully replicated vdi with x copies
+# @erasure-codedCreate an erasure coded vdi with x data strips and
+#   y parity strips
+#
+# Since: 2.12
+##
+{ 'enum': 'SheepdogRedundancyType',
+  'data': [ 'full', 'erasure-coded' ] }
+
+##
+# @SheepdogRedundancyFull:
+#
+# @copies   Number of copies to use (between 1 and 31)
+#
+# Since: 2.12
+##
+{ 'struct': 'SheepdogRedundancyFull',
+  'data': { 'copies': 'int' }}
+
+##
+# @SheepdogRedundancyErasureCoded:
+#
+# @data-strips  Number of data strips to use (one of {2,4,8,16})
+# @parity-stripsNumber of parity strips to use (between 1 and 15)
+#
+# Since: 2.12
+##
+{ 'struct': 'SheepdogRedundancyErasureCoded',
+  'data': { 'data-strips': 'int',
+'parity-strips': 'int' }}
+
+##
+# @SheepdogRedundancy:
+#
+# Since: 2.12
+##
+{ 'union': 'SheepdogRedundancy',
+  'base': { 'type': 'SheepdogRedundancyType' },
+  'discriminator': 'type',
+  'data': { 'full': 'SheepdogRedundancyFull',
+'erasure-coded': 'SheepdogRedundancyErasureCoded' } }
+
+##
 # @BlockdevCreateNotSupported:
 #
 # This is used for all drivers that don't support creating images.
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 3c3becf94d..22df2ba9d0 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1882,6 +1882,48 @@ out_with_err_set:
 return ret;
 }
 
+static int parse_redundancy(BDRVSheepdogState *s, SheepdogRedundancy *opt)
+{
+struct SheepdogInode *inode = >inode;
+
+switch (opt->type) {
+case SHEEPDOG_REDUNDANCY_TYPE_FULL:
+if (opt->u.full.copies > SD_MAX_COPIES || opt->u.full.copies < 1) {
+return -EINVAL;
+}
+inode->copy_policy = 0;
+inode->nr_copies = opt->u.full.copies;
+return 0;
+
+case SHEEPDOG_REDUNDANCY_TYPE_ERASURE_CODED:
+{
+int64_t copy = opt->u.erasure_coded.data_strips;
+int64_t parity = opt->u.erasure_coded.parity_strips;
+
+if (copy != 2 && copy != 4 && copy != 8 && copy != 16) {
+return -EINVAL;
+}
+
+if (parity >= SD_EC_MAX_STRIP || parity < 1) {
+return -EINVAL;
+}
+
+/*
+ * 4 bits for parity and 4 bits for data.
+ * We have to compress upper data bits because it can't represent 16
+ */
+inode->copy_policy = ((copy / 2) << 4) + parity;
+inode->nr_copies = copy + parity;
+return 0;
+}
+
+default:
+g_assert_not_reached();
+}
+
+return -EINVAL;
+}
+
 /*
  * Sheepdog support two kinds of redundancy, full replication and erasure
  * coding.
@@ -1892,12 +1934,13 @@ out_with_err_set:
  * # create a erasure coded vdi with x data strips and y parity strips
  * -o redundancy=x:y (x must be one of {2,4,8,16} and 1 <= y < SD_EC_MAX_STRIP)
  */
-static int parse_redundancy(BDRVSheepdogState *s, const char *opt)
+static int parse_redundancy_str(BDRVSheepdogState *s, const char *opt)
 {
-struct SheepdogInode *inode = >inode;
+struct SheepdogRedundancy redundancy;
 const char *n1, *n2;
 long copy, parity;
 char p[10];
+int ret;
 
 pstrcpy(p, sizeof(p), opt);
 n1 = strtok(p, ":");
@@ -1907,35 +1950,32 @@ static int parse_redundancy(BDRVSheepdogState *s, const 
char *opt)
 return -EINVAL;
 }
 
-copy = strtol(n1, NULL, 10);
-/* FIXME fix error checking by switching to qemu_strtol() */
-if (copy > SD_MAX_COPIES || copy < 1) {
-return -EINVAL;
-}
-if (!n2) {
-inode->copy_policy = 0;
-inode->nr_copies = copy;
-return 0;
+ret = qemu_strtol(n1, NULL, 10, );
+if (ret < 0) {
+return ret;
 }
 
-if (copy != 2 && copy != 4 && copy != 8 && copy != 16) {
-return -EINVAL;
-}
+if (!n2) {
+redundancy = (SheepdogRedundancy) {
+.type   = SHEEPDOG_REDUNDANCY_TYPE_FULL,
+.u.full.copies  = copy,
+};
+} else {
+ret = qemu_strtol(n2, NULL, 10, );
+if (ret < 0) {
+return ret;
+}
 
-parity = strtol(n2, NULL, 10);
-/* FIXME fix error checking by