On 06.05.2020 18:44, Paul Durrant wrote:
>> From: Jan Beulich <jbeul...@suse.com>
>> Sent: 29 April 2020 12:02
>>
>> On 07.04.2020 19:38, Paul Durrant wrote:
>>> +int domain_load_begin(struct domain_context *c, unsigned int tc,
>>> + const char *name, const struct vcpu *v, size_t len,
>>> + bool exact)
>>> +{
>>> + if ( c->log )
>>> + gdprintk(XENLOG_INFO, "%pv load: %s (%lu)\n", v, name,
>>> + (unsigned long)len);
>>> +
>>> + BUG_ON(tc != c->desc.typecode);
>>> + BUG_ON(v->vcpu_id != c->desc.vcpu_id);
>>> +
>>> + if ( (exact && (len != c->desc.length)) ||
>>> + (len < c->desc.length) )
>>> + return -EINVAL;
>>
>> How about
>>
>> if ( exact ? len != c->desc.length
>> : len < c->desc.length )
>>
>
> Yes, that doesn't look too bad.
>
>> ? I'm also unsure about the < - don't you mean > instead? Too
>> little data would be compensated by zero padding, but too
>> much data can't be dealt with. But maybe I'm getting the sense
>> of len wrong ...
>
> I think the < is correct. The caller needs to have at least enough
> space to accommodate the context record.
But this is load, not save - the caller supplies the data. If
there's less data than can be fit, it'll be zero-extended. If
there's too much data, the excess you don't know what to do
with (it might be okay to tolerate it being all zero).
>>> + if ( (!handlers[i].per_vcpu && c.desc.vcpu_id) ||
>>> + (c.desc.vcpu_id >= d->max_vcpus) )
>>> + break;
>>> +
>>> + v = d->vcpu[c.desc.vcpu_id];
>>> +
>>> + if ( flags & DOMAIN_SAVE_FLAG_IGNORE )
>>> + {
>>> + /* Sink the data */
>>> + rc = domain_load_entry(&c, c.desc.typecode, "IGNORED",
>>> + v, NULL, c.desc.length, true);
>>
>> IOW the read handlers need to be able to deal with a NULL dst?
>> Sounds a little fragile to me. Is there a reason
>> domain_load_data() can't skip the call to read()?
>
> Something has to move the cursor so sinking the data using a
> NULL dst seemed like the best way to avoid the need for a
> separate callback function.
And domain_load_data() can't itself advance the cursor in such
a case, with no callback involved at all?
>>> + uint16_t typecode;
>>> + /*
>>> + * Each entry will contain either to global or per-vcpu domain state.
>>> + * Entries relating to global state should have zero in this field.
>>
>> Is there a reason to specify zero?
>>
>
> Not particularly but I thought it best to at least specify a value in all
> cases.
A specific value is certainly a good idea; I wonder though whether
an obviously invalid one (like ~0) wouldn't be better then,
allowing this ID to later be assigned meaning in such cases if
need be.
>>> + */
>>> + uint16_t vcpu_id;
>>
>> Seeing (possibly) multi-instance records in HVM state (PIC, IO-APIC, HPET),
>> wouldn't it be better to generalize this to ID, meaning "vCPU ID" just for
>> per-vCPU state?
>
> True, a generic 'instance' would be needed for such records. I'll so as you
> suggest.
Which, along with my comment on domain_save_begin() taking a
struct vcpu * right now, will then indeed require changing
to a (struct domain *, unsigned int instance) tuple, I guess.
>>> +#define DOMAIN_SAVE_BEGIN(_x, _c, _v, _len) \
>>> + domain_save_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_v), (_len))
>>
>> In new code I'd like to ask for no leading underscores on macro
>> parameters as well as no unnecessary parenthesization of macro
>> parameters (e.g. when they simply get passed on to another macro
>> or function without being part of a larger expression).
>
> Personally I think it is generally good practice to parenthesize
> but I can drop if you prefer.
To be honest - it's more than just "prefer": Excess parentheses
hamper readability. There shouldn't be too few, and since macros
already require quite a lot of them, imo we should strive to
have exactly as many as are needed.
Jan
