[libvirt] [PATCH 4/4] util: fix removal of callbacks in virCloseCallbacksRun

[Michael Chapman]

On Tue, 7 Apr 2015, Ján Tomko wrote:
> On Mon, Mar 30, 2015 at 01:41:01PM +1100, Michael Chapman wrote:
>> The close callbacks hash are keyed by a UUID-string, but
>> virCloseCallbacksRun was attempting to remove them by raw UUID. This
>> patch ensures the callback entries are removed by UUID-string as well.
>>
>> This bug could cause problems when guest migrations were abnormally aborted:
>>
>>   # timeout --signal KILL 1 \
>>       virsh migrate example qemu+tls://remote/system \
>>         --verbose --compressed --live --auto-converge \
>>         --abort-on-error --unsafe --persistent \
>>         --undefinesource --copy-storage-all --xml example.xml
>>   Killed
>>
>>   # virsh migrate example qemu+tls://remote/system \
>>       --verbose --compressed --live --auto-converge \
>>       --abort-on-error --unsafe --persistent \
>>       --undefinesource --copy-storage-all --xml example.xml
>>   error: Requested operation is not valid: domain 'example' is not being migrated
>>
>> Signed-off-by: Michael Chapman <mike at very.puzzling.org>
>> ---
>>  src/util/virclosecallbacks.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c
>> index 4128057..b0a1a9e 100644
>> --- a/src/util/virclosecallbacks.c
>> +++ b/src/util/virclosecallbacks.c
>> @@ -236,6 +236,7 @@ typedef struct _virCloseCallbacksListEntry virCloseCallbacksListEntry;
>>  typedef virCloseCallbacksListEntry *virCloseCallbacksListEntryPtr;
>>  struct _virCloseCallbacksListEntry {
>>      unsigned char uuid[VIR_UUID_BUFLEN];
>> +    char uuidstr[VIR_UUID_STRING_BUFLEN];
>
> We shouldn't be storing the uuid twice.

OK.

>>      virCloseCallback callback;
>>  };
>>
>> @@ -279,6 +280,8 @@ virCloseCallbacksGetOne(void *payload,
>>
>>      memcpy(data->list->entries[data->list->nentries - 1].uuid,
>>             uuid, VIR_UUID_BUFLEN);
>> +    memcpy(data->list->entries[data->list->nentries - 1].uuidstr,
>> +           uuidstr, VIR_UUID_STRING_BUFLEN);
>>      data->list->entries[data->list->nentries - 1].callback = closeDef->cb;
>>  }
>>
>> @@ -332,7 +335,7 @@ virCloseCallbacksRun(virCloseCallbacksPtr closeCallbacks,
>>
>>      for (i = 0; i < list->nentries; i++) {
>>          virHashRemoveEntry(closeCallbacks->list,
>> -                           list->entries[i].uuid);
>> +                           list->entries[i].uuidstr);
>
> I think the CPU time wasted by doing virUUIDFormat here again is
> negligible and easier than writing a Hash function that works on
> VIR_UUID buffers :)
>
> Jan

Yeah, sounds reasonable. I'll send through a v2 patch shortly. I've also 
got a few extra patches lined up, but I'll start a new series for those.

Thanks for the review!

- Michael