On 05/19/2017 07:47 PM, Dr. David Alan Gilbert wrote: > * Halil Pasic (pa...@linux.vnet.ibm.com) 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 / dgilb...@redhat.com / Manchester, UK >