[libvirt] [PATCH] esx: Fix dynamic dispatch for CastFromAnyType functions

[Matthias Bolte]

2011/4/27 Daniel Veillard <veillard at redhat.com>:
> On Mon, Apr 25, 2011 at 01:32:43PM +0200, Matthias Bolte wrote:
>> 2011/4/25 Matthias Bolte <matthias.bolte at googlemail.com>:
>> > This was broken by the refactoring in ac1e6586ec75. It resulted in a
>> > segfault for 'virsh vol-dumpxml' and related volume functions.
>> >
>>
>> Actually that patch doesn't fix the problem correctly. It just turns
>> the segfault into an error message.
>>
>> Here's v2 that really fixes the problem. Note the important difference
>> between anyType->_type and anyType->type :)
>>
>> Matthias
>
>> From a17245fb92c88439ffbb84e34bebf1afb6903885 Mon Sep 17 00:00:00 2001
>> From: Matthias Bolte <matthias.bolte at googlemail.com>
>> Date: Mon, 25 Apr 2011 12:38:17 +0200
>> Subject: [PATCH] esx: Fix dynamic dispatch for CastFromAnyType functions
>>
>> This was broken by the refactoring in ac1e6586ec75. It resulted in a
>> segfault for 'virsh vol-dumpxml' and related volume functions.
>>
>> Before the refactoring all users of the ESX_VI__TEMPLATE__DISPATCH
>> macro dispatched on the item type, as the item is the input to all those
>> functions.
>>
>> Commit ac1e6586ec75 made the dynamically dispatched CastFromAnyType
>> functions use this macro too, but this functions dispatched on the
>> actual type of the AnyType object. The item is the output of the
>> CastFromAnyType functions.
>>
>> This difference was missed in the refactoring, making CastFromAnyType
>> functions dereferencing the item pointer that is NULL at the time of
>> the dispatch.
>> ---
>>  src/esx/esx_vi_types.c |   14 ++++++++------
>>  1 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/esx/esx_vi_types.c b/src/esx/esx_vi_types.c
>> index dd83954..3c81021 100644
>> --- a/src/esx/esx_vi_types.c
>> +++ b/src/esx/esx_vi_types.c
>> @@ -533,8 +533,9 @@
>>   * Macros to implement dynamic dispatched functions
>>   */
>>
>> -#define ESX_VI__TEMPLATE__DISPATCH(__type, _dispatch, _error_return)          \
>> -    switch (item->_type) {                                                    \
>> +#define ESX_VI__TEMPLATE__DISPATCH(_actual_type, __type, _dispatch,           \
>> +                                   _error_return)                             \
>> +    switch (_actual_type) {                                                   \
>>        _dispatch                                                               \
>>                                                                                \
>>        case esxVI_Type_##__type:                                               \
>> @@ -543,7 +544,7 @@
>>        default:                                                                \
>>          ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR,                                  \
>>                       _("Call to %s for unexpected type '%s'"), __FUNCTION__,  \
>> -                     esxVI_Type_ToString(item->_type));                       \
>> +                     esxVI_Type_ToString(_actual_type));                      \
>>          return _error_return;                                                 \
>>      }
>>
>> @@ -585,7 +586,8 @@
>>
>>  #define ESX_VI__TEMPLATE__DYNAMIC_FREE(__type, _dispatch, _body)              \
>>      ESX_VI__TEMPLATE__FREE(__type,                                            \
>> -      ESX_VI__TEMPLATE__DISPATCH(__type, _dispatch, /* nothing */)            \
>> +      ESX_VI__TEMPLATE__DISPATCH(item->_type, __type, _dispatch,              \
>> +                                 /* nothing */)                               \
>>        _body)
>>
>>
>> @@ -618,14 +620,14 @@
>>
>>  #define ESX_VI__TEMPLATE__DYNAMIC_CAST_FROM_ANY_TYPE(__type, _dispatch)       \
>>      ESX_VI__TEMPLATE__CAST_FROM_ANY_TYPE_EXTRA(__type, esxVI_##__type,        \
>> -      ESX_VI__TEMPLATE__DISPATCH(__type, _dispatch, -1),                      \
>> +      ESX_VI__TEMPLATE__DISPATCH(anyType->type, __type, _dispatch, -1),       \
>>        /* nothing */)
>>
>>
>>
>>  #define ESX_VI__TEMPLATE__DYNAMIC_SERIALIZE(__type, _dispatch, _serialize)    \
>>      ESX_VI__TEMPLATE__SERIALIZE_EXTRA(__type,                                 \
>> -      ESX_VI__TEMPLATE__DISPATCH(__type, _dispatch, -1),                      \
>> +      ESX_VI__TEMPLATE__DISPATCH(item->_type, __type, _dispatch, -1),         \
>>        _serialize)
>>
>
>  Not sure I fully understand but looks regular and okay, ACK :-)
>
> Daniel
>

Actually, this shows that even I'm not always aware of all the details
in the ESX driver, otherwise this regression fix wouldn't have be
necessary and I wouldn't have needed two attempts to get it right. So
don't feel too bad about not fully understanding it :)

This is probably a sign that I should try to reduce the complexity and
levels of indirection in the generated code and the code generator.

Thanks, finally pushed v2.

Matthias