On 09/11/2013 02:27 PM, Paolo Bonzini wrote: > Il 11/09/2013 13:06, Juan Quintela ha scritto: >>>> And I think that the right solution is make qemu_get_rate_limit() to >>>> return -1 in case of error (or the error, I don't care). >>> >>> You might do both things, it would avoid the useless g_usleep you >>> pointed out below. But Lei's patch is good, because an error could >>> happen exactly during the qemu_put_be64 that writes RAM_SAVE_FLAG_EOS. >> >> Caller checks also. This is the reason I wanted qemu_file_* callers to >> return an error. It has some advantages and some disadvantages. We >> don't agree on which ones are bigger O:-) > > I think the disadvantages are bigger. It litters the code with error > handling, hides where things actually happen, and doesn't even simplify > QEMUFile itself. Checking only at the toplevel is simpler, all we need > to do is ensure that we get there every now and then (and that's what > qemu_file_rate_limit does). >
I also prefer the error checking at the top level. Orit >>>> savevm.c: qemu_savevm_state_iterate() >>>> >>>> if (qemu_file_rate_limit(f)) { >>>> return 0; >>>> } >>>> >>>> check is incorrect again, we should return an error if there is one >>>> error. >>> >>> Nothing cares if qemu_savevm_state_iterate returns 0 or negative, so >>> changing qemu_savevm_state_iterate to only return 0/1 would make sense too. >> >> In this case, 0 means: >> please, call us again >> when what we mean is: >> don't care about calling us again, there is an error. Handle the error. > > Or alternatively, 0 means: > > we haven't finished the work > > when what we mean is: > > we haven't finished the work (BTW, please check if there is an error) > >> Notice that qemu_save_iterate() already returns errors in other code >> paths > > Yes that's also unnecessary. > >> If we change th ereturn value for qemu_file_rate_limit() the change that >> cames with this patch is not needed, that was my point. > > This is what an earlier patch from Lei did. I told him (or her?) to > leave qemu_file_rate_limit aside since the idea behind QEMUFile is to > only handle the error at the top. > > Paolo >