* Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
>
>
> On 07/13/2017 05:11 PM, Dr. David Alan Gilbert wrote:
> > * Christian Borntraeger (borntrae...@de.ibm.com) wrote:
> >> On 07/13/2017 04:49 PM, Dr. David Alan Gilbert wrote:
> >>> * Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> >>>>
> >>>>
> >>>> On 07/13/2017 02:27 PM, Cornelia Huck wrote:
> >>>>>> +static void kvm_flic_ais_pre_save(void *opaque)
> >>>>>> +{
> >>>>>> + KVMS390FLICStateMigTmp *tmp = opaque;
> >>>>>> + KVMS390FLICState *flic = tmp->parent;
> >>>>>> + struct kvm_s390_ais_all ais;
> >>>>>> + struct kvm_device_attr attr = {
> >>>>>> + .group = KVM_DEV_FLIC_AISM_ALL,
> >>>>>> + .addr = (uint64_t)&ais,
> >>>>>> + .attr = sizeof(ais),
> >>>>>> + };
> >>>>>> +
> >>>>>> + if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) {
> >>>>>> + error_report("Failed to retrieve kvm flic ais states");
> >>>>> There's not much else we can do in that case, is there?
> >>>>>
> >>>>
> >>>> I think this is a very good question! The ioctl should not fail
> >>>> under any circumstances, but if it does we have a problem.
> >>>>
> >>>> Carrying on happily (what we do now) means effectively discarding
> >>>> ais state. In general just discarding state ain't a good idea.
> >>>>
> >>>> In particular it might be OK, but the patch should explain that!
> >>>>
> >>>> Regarding what could/should we do in such a case (instead
> >>>> of discarding state and carrying on happily) I don't know, so
> >>>> I tend to agree with you regarding 'not much else we can do'.
> >>>>
> >>>> Adding Dave and Juan. Maybe they can tell.
> >>>
> >>> I keep meaning to make the pre_save give a return value for failure,
> >>> but it hasn't currently got one.
> >>
> >> Would you accept patches for that?
> >
> > Sure.
> >
> >>>
> >>> You could try something like:
> >>>
> >>> qemu_file_set_error(migrate_get_current()->to_dst_file, -EINVAL);
>
> @Dave:
> Thanks Dave! I was not aware of that! Had a quick look at the
> code, I think qemu_file_set_error would indeed do the right thing.
>
> I would prefer error handling being part of the pre_save interface,
> because that would be easier to understand, and would provoke thinking
> about these problems.
>
> @Christian:
> Would you like to implement 'return value for pre_save'
> yourself? I mean, I the meanwhile I'm familiar with the code in question
> and I enjoy working with Dave and Juan, so if you aren't interested in
> doing it yourself but think it's important enough to get it done, I could
> take it too?
>
> @Dave:
> There are a couple of questions I'm gonna have to ask/investigate should
> it be me doing the 'return value for pre_save' (also notes to myself):
>
> Would you see this error handling via pre_save as a parallel infrastructure
> (keep the current qemu_file_set_error mechanism) or would you prefer
> things converted? IMHO having a single method would be cleaner, but I
> have not looked into this in great detail.
The only thing I'd like to change is make pre_save be:
int (*pre_save)(void *opaque)
rather than void.
If there are any current pre_save's that call set_error or assert or
anything like that then they could be converted, but I don't think there
are many.
> Also the question what is the semantic of qemu_file_set_error arises.
> It ain't documented and I would intuitively suspect that it's rather
> about the 'file' (that is transport) than the whole migration.
It's really an internal interface in migration (where it's not
very nice either); but really it shouldn't be used for anything else.
Dave
>
>
> >>>
> >>> I *think* the migration code should spot that before it finishes
> >>> but it might carry on for a little while before it does.
> >>
> >> I will keep this patch as is, since this is one of the "should not happen"
> >> cases.
>
> @Christian
> I'm OK with it, because knowing the kernel code behind the ioctl
> this is really unlikely and even if it should happen the risks involved
> are rather limited. But I would be much happier if all such
> cases would result in refusing migration.
>
> Regards,
> Halil
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
