Re: [Qemu-devel] [PATCH v2 1/1] include: Auto-generate the sizes lookup table

[Eric Blake]

On 1/8/19 3:31 AM, Markus Armbruster wrote:
> Copying our resident shell script guru Eric.

Who already expressed a desire to keep things hard-coded on v1 ;)

And I've also already expressed a dislike for the entire S_XXXiB macro
list, thinking that a nicer solution would instead be teaching QemuOpt
that instead of encoding all defaults as strings (where we are using
S_XXXiB strings in order to work around the fact that we have to store
the stringized value of an integer as the default), we should instead
encode defaults as an appropriate type (and then do a runtime conversion
of that type into a string when displaying the help output that shows
the default value).  But I don't know who wants to sign up for that
work, as that is already putting lipstick on a pig (we'd rather be using
Markus' work on converting command-line parsing to QAPI, rather than
expanding QemuOpts even more).  The more work we put into the S_XXXiB
table, the harder it will be to rip it out later when we do come up with
a nicer solution that does not require users to store integer defaults
as a correctly-spelled string.

> 
> Leonid Bloch <address@hidden> writes:
> 
>> The lookup table for power-of-two sizes is now auto-generated during the
>> build, and not hard-coded into the units.h file.
>>
>> This partially reverts commit 540b8492618eb.
>>

> 
> I'd leave it hard-coded.  Replacing a few trivial defines by an arguably
> less trivial script doesn't feel like an improvement.  In this case, it
> doesn't even save lines.  But I'm not the maintainer.

That's several people that have leaned towards not having a generator
script.


>> +++ b/include/qemu/units.h
>> @@ -17,77 +17,4 @@
>>  #define PiB     (INT64_C(1) << 50)
>>  #define EiB     (INT64_C(1) << 60)
>>  
>> -/*
>> - * The following lookup table is intended to be used when a literal string 
>> of
>> - * the number of bytes is required (for example if it needs to be 
>> stringified).
>> - * It can also be used for generic shortcuts of power-of-two sizes.
>> - * This table is generated using the AWK script below:
>> - *
>> - *  BEGIN {
>> - *      suffix="KMGTPE";
>> - *      for(i=10; i<64; i++) {
>> - *          val=2**i;
>> - *          s=substr(suffix, int(i/10), 1);
>> - *          n=2**(i%10);
>> - *          pad=21-int(log(n)/log(10));
>> - *          printf("#define S_%d%siB %*d\n", n, s, pad, val);
>> - *      }
>> - *  }
> 
> If we decide not keep the defines hard-coded, I think we can delete the
> AWK script.

Kevin also pointed out that if we keep things hard-coded, we still need
to fix the hard-coding to use correct types for the second half of the
table:

>> -#define S_4GiB            4294967296
>> -#define S_8GiB            8589934592

and even if we write a generator instead of hard-coding, our generator
also needs to make that change, which makes the generator a bit more
complicated.

>> +++ b/scripts/gen-sizes.sh
> 
> I'm not sure I'd use shell for this, but since you already wrote it and
> it works...
> 
>> @@ -0,0 +1,66 @@
>> +#!/bin/sh

Note that this selects 'sh',...

>> +
>> +size_suffix() {
>> +    case ${1} in
>> +        1)
>> +            printf "KiB"
>> +            ;;
>> +        2)
>> +            printf "MiB"
>> +            ;;
>> +        3)
>> +            printf "GiB"
>> +            ;;
>> +        4)
>> +            printf "TiB"
>> +            ;;
>> +        5)
>> +            printf "PiB"
>> +            ;;
>> +        6)
>> +            printf "EiB"
>> +            ;;
>> +    esac
>> +}
> 
> Terser:
> 
>        suf=('' 'K' 'M' 'G' 'T' 'P' 'E')
>        printf "${suf[$1]}iB"

...but this terser representation depends on a bash-ism, so if we did go
with a generator, you'd have to fix the shebang to be bash.  The list
starts at S_1KiB, so the '' entry in suf is being used as a filler to
get indexing to work as desired.

Or, sticking to POSIX shell, you could do a similar lookup using sed:

echo XKMGTPE | sed "s/^.\{$1\}\(.\).*/\1/"

> 
>> +
>> +print_sizes() {
>> +    local p=10

local is a bash-ism. So your script is already dependent on bash, unless
you drop the use of local, even without the above paragraph on
shortening the case statement into a one-liner.

>> +    while [ ${p} -lt 64 ]
>> +    do
>> +        local pad=' '
>> +        local n=$((p % 10))
>> +        n=$((1 << n))
>> +        [ $((n / 100)) -eq 0 ] && pad='  '
>> +        [ $((n / 10)) -eq 0 ] && pad='   '
>> +        local suff=$((p / 10))
>> +        printf "#define S_%u%s%s%20u\n" ${n} "$(size_suffix ${suff})" \
>> +            "${pad}" $((1 << p))
>> +        p=$((p + 1))
>> +    done
> 
> Rule of thumb: when you compute blank padding strings, you're not fully
> exploiting printf :)
> 
>     local p
>     for ((p=10; p < 64; p++))
>     do
>         local n=$((1 << (p % 10)))
>       local sym=$(printf "S_%u%s" $n "$(size_suffix $((p / 10)))")
>         printf "#define %-8s %20u\n" $sym $((1 << p))
>     done
> 
>> +}
>> +
>> +print_header() {
>> +    cat <<EOF
>> +/* AUTOMATICALLY GENERATED, DO NOT MODIFY.
>> + *
>> + * The following lookup table is intended to be used when a literal string 
>> of
>> + * the number of bytes is required (for example if it needs to be 
>> stringified).
>> + * It can also be used for generic shortcuts of power-of-two sizes.
>> + *
>> + * Authors:
>> + *   Leonid Bloch  <address@hidden>
>> + */

Do we still need Authors: lines in files, or can we just trust git
history (which is going to be more accurate anyway)?

>> +
>> +#ifndef QEMU_SIZES_H
>> +#define QEMU_SIZES_H
>> +
>> +EOF
>> +}
>> +
>> +print_footer() {
>> +    printf "\n#endif  /* QEMU_SIZES_H */\n"
>> +}
>> +
>> +print_header  > "${1}"
>> +print_sizes  >> "${1}"
>> +print_footer >> "${1}"
> 
> Unchecked use of command-line arguments is not nice:
> 
>     $ scripts/gen-sizes.sh 
>     scripts/gen-sizes.sh: line 64: : No such file or directory
>     scripts/gen-sizes.sh: line 65: : No such file or directory
>     scripts/gen-sizes.sh: line 66: : No such file or directory
> 
> You should error out if $# -ne 1.  But in such a simple script, I'd
> dispense with arguments and print to stdout.  Matter of taste.
> Rejecting $# -ne 0 is still nice then.

Also, do you really need to break the work into functions that get
called exactly once?  Just do the work directly.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature