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

[Halil Pasic]

On 05/19/2017 07:47 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (address@hidden) wrote:
>>
>>
>> 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.
> 
> Lets go with what you put in the other patch (I replied to it); I hadn't
> realised that was possible (hence my comment below).
> Once we have a bunch of different uses of VMSTATE_WITH_TMP in the code
> base, I'll step back and see how to tidy them up.
> 
> Dave
> 

Sounds very reasonable. Let's do it like that!

Halil

>>>
>>> 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
>>
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
>