Re: [Qemu-devel] [PATCH v5 2/2] sheepdog: support user-defined redundancy option

[Stefan Hajnoczi]

On Fri, Nov 01, 2013 at 11:10:13PM +0800, Liu Yuan wrote:
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 66b3ea8..a267d31 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -91,6 +91,14 @@
>  #define SD_NR_VDIS   (1U << 24)
>  #define SD_DATA_OBJ_SIZE (UINT64_C(1) << 22)
>  #define SD_MAX_VDI_SIZE (SD_DATA_OBJ_SIZE * MAX_DATA_OBJS)
> +/*
> + * For erasure coding, we use at most SD_EC_MAX_STRIP for data strips and
> + * (SD_EC_MAX_STRIP - 1) for parity strips
> + *
> + * SD_MAX_COPIES is sum of number of data trips and parity strips.

s/data trips/data strips/

> +static bool is_numeric(const char *s)
> +{
> +    const char *p = s;
> +
> +    if (*p) {
> +        char c;
> +
> +        while ((c = *p++))
> +            if (!isdigit(c)) {
> +                return false;
> +            }
> +        return true;
> +    }
> +    return false;
> +}
> +
> +/*
> + * Sheepdog support two kinds of redundancy, full replication and erasure
> + * coding.
> + *
> + * # create a fully replicated vdi with x copies
> + * -o redundancy=x (1 <= x <= SD_MAX_COPIES)
> + *
> + * # 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)
> +{
> +    struct SheepdogInode *inode = &s->inode;
> +    const char *n1, *n2;
> +    uint8_t copy, parity;
> +    char p[10];
> +
> +    pstrcpy(p, sizeof(p), opt);
> +    n1 = strtok(p, ":");
> +    n2 = strtok(NULL, ":");
> +
> +    if (!n1 || !is_numeric(n1) || (n2 && !is_numeric(n2))) {
> +        return -EINVAL;
> +    }
> +
> +    copy = strtol(n1, NULL, 10);
> +    if (copy > SD_MAX_COPIES) {
> +        return -EINVAL;
> +    }
> +    if (!n2) {
> +        inode->copy_policy = 0;
> +        inode->nr_copies = copy;
> +        return 0;
> +    }
> +
> +    if (copy != 2 && copy != 4 && copy != 8 && copy != 16) {
> +        return -EINVAL;
> +    }
> +
> +    parity = strtol(n2, NULL, 10);
> +    if (parity >= SD_EC_MAX_STRIP || parity == 0) {
> +        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;
> +}

The string manipulation can be simplified using sscanf(3) and
is_numeric() can be dropped:

static int parse_redundancy(BDRVSheepdogState *s, const char *opt)
{
    struct SheepdogInode *inode = &s->inode;
    uint8_t copy, parity;
    int n;

    n = sscanf(opt, "%hhu:%hhu", &copy, &parity);
    if (n != 1 && n != 2) {
        return -EINVAL;
    }
    if (copy > SD_MAX_COPIES) {
        return -EINVAL;
    }
    if (n == 1) {
        inode->copy_policy = 0;
        inode->nr_copies = copy;
        return 0;
    }
    if (copy != 2 && copy != 4 && copy != 8 && copy != 16) {
        return -EINVAL;
    }
    if (parity >= SD_EC_MAX_STRIP || parity == 0) {
        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;
}