On 17.01.2023 19:48, Andrew Cooper wrote:
> On 11/01/2023 1:54 pm, Jan Beulich wrote:
>> First of all move the almost loop-invariant condition out of the loop;
>> transform it into an altered loop boundary. Since the new local variable
>> wants to be "unsigned int" and named without violating name space rules,
>> convert the loop induction variable accordingly.
>
> I'm firmly -1 against using trailing underscores.
Well, I can undo that aspect, but just to get done with the change. I do
consider ...
> Mainly, I object to the attempt to justify doing so based on a rule we
> explicitly choose to violate for code consistency and legibility reasons.
... your view here at least questionable: I'm unaware of us doing so
explicitly, and I've pointed out numerous times that the C standard
specifies very clearly what underscore-prefixed names may be used for.
> But in this case, you're taking a block of logic which was cleanly in
> one style, and making it mixed, even amongst only the local variables.
That's simply the result of wanting to limit how much of a change I
make to the macro, such that the actual changes are easier to spot.
>> --- a/xen/arch/x86/mm/shadow/multi.c
>> +++ b/xen/arch/x86/mm/shadow/multi.c
>> @@ -863,23 +863,20 @@ do {
>> /* 64-bit l2: touch all entries except for PAE compat guests. */
>> #define SHADOW_FOREACH_L2E(_sl2mfn, _sl2e, _gl2p, _done, _dom, _code)
>> \
>> do {
>> \
>> - int _i;
>> \
>> - int _xen = !shadow_mode_external(_dom);
>> \
>> + unsigned int i_, end_ = SHADOW_L2_PAGETABLE_ENTRIES;
>> \
>> shadow_l2e_t *_sp = map_domain_page((_sl2mfn));
>> \
>> ASSERT_VALID_L2(mfn_to_page(_sl2mfn)->u.sh.type);
>> \
>> - for ( _i = 0; _i < SHADOW_L2_PAGETABLE_ENTRIES; _i++ )
>> \
>> + if ( !shadow_mode_external(_dom) &&
>> \
>> + is_pv_32bit_domain(_dom) &&
>> \
>
> The second clause here implies the first. Given that all we're trying
> to say here is "are there Xen entries to skip", I think we'd be fine
> dropping the external() check entirely.
Will do. I may retain this in some form of comment.
>> + mfn_to_page(_sl2mfn)->u.sh.type != SH_type_l2_64_shadow )
>> \
>> + end_ = COMPAT_L2_PAGETABLE_FIRST_XEN_SLOT(_dom);
>> \
>> + for ( i_ = 0; i_ < end_; ++i_ )
>> \
>> {
>> \
>> - if ( (!(_xen))
>> \
>> - || !is_pv_32bit_domain(_dom)
>> \
>> - || mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_64_shadow
>> \
>> - || (_i < COMPAT_L2_PAGETABLE_FIRST_XEN_SLOT(_dom)) )
>> \
>> - {
>> \
>> - (_sl2e) = _sp + _i;
>> \
>> - if ( shadow_l2e_get_flags(*(_sl2e)) & _PAGE_PRESENT )
>> \
>> - {_code}
>> \
>> - if ( _done ) break;
>> \
>> - increment_ptr_to_guest_entry(_gl2p);
>> \
>> - }
>> \
>> + (_sl2e) = _sp + i_;
>> \
>> + if ( shadow_l2e_get_flags(*(_sl2e)) & _PAGE_PRESENT )
>> \
>> + { _code }
>> \
>
> This doesn't match either of our two styles.
Indeed, and I was unable to come up with good criteria whether to leave
it (for consistency with the other macros) or change it. Since you're
...
> if ( ... )
> { _code }
>
> would be closer to Xen's normal style, but ...
>
>> + if ( _done ) break;
>> \
>
> ... with this too, I think it would still be better to write it out
> fully, so:
>
> if ( ... )
> {
> _code
> }
> if ( _done )
> break;
>
> These macros are already big enough that trying to save 3 lines seems
> futile.
... explicitly asking for it, I'll change then. Would you mind if I then
also added a semicolon after _code, to make things look more sensible?
Jan
