Re: [Qemu-devel] [PATCH] migration: discard RAMBlocks of type ram_device

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2180 bytes --]

On 04/12/2018 06:47 AM, Peter Maydell wrote:

>>  /*
>> + * Identifies RAM blocks which should be discarded from migration. For
>> + * the moment, it only applies to blocks backed by a 'ram_device'
>> + * memory region.
>> + */
>> +static inline bool ram_block_is_migratable(RAMBlock *block)
>> +{
>> +    return !memory_region_is_ram_device(block->mr);
>> +}
>> +
>> +/* Should be holding either ram_list.mutex, or the RCU lock. */
>> +#define RAMBLOCK_FOREACH_MIGRATABLE(block)             \
>> +    RAMBLOCK_FOREACH(block)                            \
>> +        if (ram_block_is_migratable(block))
> 
> This will mishandle some uses, like:
> 
>     if (foo)
>         RAMBLOCK_FOREACH_MIGRATABLE(block)
>             stuff;
>     else
>         morestuff;
> 
> as the if() inside the macro will capture the else clause.
> (The lack of braces in the calling code would be against our
> coding style, of course, so not very likely.)

All existing callers of RAMBLOCK_FOREACH() already supply their own {}
around stuff (same is true for QLIST_FOREACH_RCU(), which is what is
being used under the hood).  By the time you correct that to:

if (foo)
    RAMBLOCK_FOREACH_MIGRATABLE(block) {
        stuff;
    }
else
    morestuff;

then even though the outer if violates coding standard, the correct
usage of the macro with {} around stuff won't leak that there is a
trailing if.

But yeah, if you're worrying about code that omitted {}, then dealing
with omitted {} in both places is something to think about.

> 
> Eric, is there a 'standard' trick for this? I thought of
> maybe
> 
> #define RAMBLOCK_FOREACH_MIGRATABLE(block)             \
>     RAMBLOCK_FOREACH(block)                            \
>         if (!ram_block_is_migratable(block)) {} else

at which point, yes, this is a little bit safer for taking exactly one
statement without risking that a later bare 'else' could match to the
wrong unbraced 'if'.  I'm not coming up with any better ideas for
(abusing?) the syntax.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]