Re: [Xen-devel] [PATCH v2 09/13] fuzz/x86_emulate: Move all state into fuzz_state

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 10/04/2017 09:25 AM, Jan Beulich wrote:
>>>> On 25.09.17 at 16:26, <george.dunlap@xxxxxxxxxx> wrote:
>> @@ -39,6 +33,12 @@ struct fuzz_corpus
>>   */
>>  struct fuzz_state
>>  {
>> +    unsigned long options;
>> +    unsigned long cr[5];
>> +    uint64_t msr[MSR_INDEX_MAX];
>> +    struct segment_register segments[SEG_NUM];
>> +    struct cpu_user_regs regs;
>> +
>>      /* Fuzzer's input data. */
>>      struct fuzz_corpus *corpus;
>>  
>> @@ -51,6 +51,8 @@ struct fuzz_state
>>      /* Emulation ops, some of which are disabled based on corpus->options. 
>> */
>>      struct x86_emulate_ops ops;
>>  };
>> +#define DATA_OFFSET offsetof(struct fuzz_state, corpus)
>> +
>>  
> 
> Personally I think this would better be placed right between the two
> respective fields in the structure, for it to at the same time serve as
> a clear indication that it needs either changing when a field would be
> inserted there, or the insertion be done elsewhere.

That sounds like a good idea.

> Also please don't
> add another blank line here.

Ack.

>> @@ -760,12 +761,11 @@ static void disable_hooks(struct x86_emulate_ctxt 
>> *ctxt)
>>  static void sanitize_input(struct x86_emulate_ctxt *ctxt)
>>  {
>>      struct fuzz_state *s = ctxt->data;
>> -    struct fuzz_corpus *c = s->corpus;
>> -    struct cpu_user_regs *regs = &c->regs;
>> -    unsigned long bitmap = c->options;
>> +    struct cpu_user_regs *regs = ctxt->regs;
> 
> I think this would more obviously look like the equivalent of the old
> code when being &s->regs, but the net effect is the same afaict, so it
> doesn't really matter.
> 
> In any event (with at least the extra blank line removed)
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks,
 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel