Re: [Xen-devel] [PATCH v3] dm_op: Add xendevicemodel_modified_memory_bulk.

[Jennifer Herbert <Jennifer.Herbert@xxxxxxxxxx>]

On 29/03/17 11:38, Jan Beulich wrote:
> > > > On 28.03.17 at 15:18, <jennifer.herbert@xxxxxxxxxx> wrote:
> > Perhaps drop "already"? Personally I also wouldn't mind you dropping 
> > the variable altogether and using header->opaque directly, but I 
> > guess that's too "opaque" for your taste? 
It would make the code too opaque for my taste, so I'll just drop the 
'already' bit.
> > @@ -441,13 +481,8 @@ static int dm_op(domid_t domid,
> >           struct xen_dm_op_modified_memory *data =
> >               &op.u.modified_memory;
> >   
> > -        const_op = false;
> > -
> > -        rc = -EINVAL;
> > -        if ( data->pad )
> > -            break;
> > -
> > -        rc = modified_memory(d, data);
> > +        rc = modified_memory(d, data, &bufs[1]);
> > +        const_op = (rc != 0);
> Isn't this wrong now, i.e. don't you need to copy back the
> header now in all cases?
I only define what I'll set nr_extents to in case of error, and of 
course opaque
is opaque.
If I where to write back, I'd be writing back 0 to nr_extents - which 
wouldn’t really
mean anything since I’m not defining the order for which I’m processing 
them.
In fact the only thing it tells you is that extent 0 is the last one 
processed, which
I don't think its all that useful.

Ideally I'd prefer to leave it untouched on success, but the original 
value is lost on
continuation, this would be more involved.

By only writing back on error, I hoped to improve efficiency for the 
common case,
(especially for existing use with calls of one extent).  (I know its 
only a small difference)
If you want me to write back - what do you want me to write back for 
success?

Cheers,

-jenny


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