qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css


From: Halil Pasic
Subject: Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css
Date: Fri, 19 May 2017 18:33:52 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0


On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote:
>> We could also consider making WITH_TMP act as a normal field. 
>> Working on the whole state looks like a bit like a corner case:
>> we have some stuff adjacent in the migration stream, and we have
>> to map it on multiple fields (and vice-versa). Getting the whole
>> state with a pointer to a certain field could work via container_of.
> You do need to know which field you're working on to be able to safely
> use container_of, so I'm not sure how it would work for you in this
> case.


Well, if you have to write to just one field you are good because you
already have a pointer to that field (.offset was added).

If you need to write to multiple fields in post_load then you just pick
one of the fields you are going to write to (probably the first) and use
container_of to gain access to the whole state. The logic is specific to
the bunch of the fields you are going to touch anyway.

In fact any member of the state struct will do it's only important that
you use the same when creating the VMStateField and when trying to get a
pointer to the parent in pre_save and post_load.

I haven't tried, so I'm not 100% sure, but if you like I can try, and send
you a patch if it's viable. 

I think the key to a good solution is really what is intended and typical
usage, and what is corner case. My patch in the other reply shows that we
can do without changing the ways of VMSTATE_WITH_TMP. I think we can make
what I'm trying to do here a bit prettier at the expense of making what
you are doing in virtio-net a bit uglier, but whether it's a good idea to
do so, I cant tell.

> 
> The other thought I'd had was that perhaps we could change the temporary
> structure in VMSTATE_WITH_TMP to:
> 
>   struct foo {
>      struct whatever **parent;
> 
> so now you could write to *parent in cases like these.
>

Sorry, I do not get your idea. If you have some WIP patch in this
direction I would be happy to provide some feedback.

 
>> Btw, I would rather call it get_indicator a factory method or even a
>> constructor than an allocator, but I think we understand each-other
>> anyway.
> Yes; I'm not too worried about the actual name as long as it's short
> and obvious.
> 
> I'd thought of 'allocator' since in most cases it's used where the
> load-time code allocates memory for the object being loaded.
> A constructor is normally something I think of as happening after
> allocation; and a factory, hmm maybe.  However, if it does the right
> thing I wouldn't object to any of those names.
> 

I think we are on the same page.

Cheers,
Halil

> Dave




reply via email to

[Prev in Thread] Current Thread [Next in Thread]