4:41 a.m.
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 | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/esx/esx_vi_types.c b/src/esx/esx_vi_types.c
index dd83954..6a2d5cf 100644
--- a/src/esx/esx_vi_types.c
+++ b/src/esx/esx_vi_types.c
@@ -533,8 +533,8 @@
* Macros to implement dynamic dispatched functions
*/
-#define ESX_VI__TEMPLATE__DISPATCH(__type, _dispatch, _error_return) \
- switch (item->_type) { \
+#define ESX_VI__TEMPLATE__DISPATCH(_object, __type, _dispatch, _error_return) \
+ switch ((_object)->_type) { \
_dispatch \
\
case esxVI_Type_##__type: \
@@ -543,7 +543,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((_object)->_type)); \
return _error_return; \
}
@@ -585,7 +585,7 @@
#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, _dispatch, /* nothing */) \
_body)
@@ -618,14 +618,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, _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, _dispatch, -1), \
_serialize)
--
1.7.0.4
6:32 a.m.
2011/4/25 Matthias Bolte <matthias.bolte(a)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
1:37 a.m.
On Mon, Apr 25, 2011 at 01:32:43PM +0200, Matthias Bolte wrote:
2011/4/25 Matthias Bolte <matthias.bolte(a)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(a)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
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
2:35 a.m.
2011/4/27 Daniel Veillard <veillard(a)redhat.com>:
On Mon, Apr 25, 2011 at 01:32:43PM +0200, Matthias Bolte wrote:
> 2011/4/25 Matthias Bolte <matthias.bolte(a)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(a)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
3:02 a.m.
On Wed, Apr 27, 2011 at 09:35:32AM +0200, Matthias Bolte wrote:
2011/4/27 Daniel Veillard <veillard(a)redhat.com>:
> 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.
Reminds me of docs/apibuild.py , that's code I wrote like 10 years
ago for libxml2, and I certainly don't remember all the subtelties,
it's certainly not very nice code, but being a generator and automated
parser the good point is that I don't have to remember it, and very
seldomly hack it. I think there is no shame to having "imperfect" code
which does this kind of things as long as it's done in a generic enought
way that you don't have to tweak it often.
If the extra indirections don't cost anything (except extra code) then
maybe it's fine keeping. I would say the main driver to clean such code
would be if you feel others need to hack on it and it need to be cleaner
and easier to understand.
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
3:37 a.m.
2011/4/27 Daniel Veillard <veillard(a)redhat.com>:
On Wed, Apr 27, 2011 at 09:35:32AM +0200, Matthias Bolte wrote:
> 2011/4/27 Daniel Veillard <veillard(a)redhat.com>:
> > 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.
Reminds me of docs/apibuild.py , that's code I wrote like 10 years
ago for libxml2, and I certainly don't remember all the subtelties,
it's certainly not very nice code, but being a generator and automated
parser the good point is that I don't have to remember it, and very
seldomly hack it. I think there is no shame to having "imperfect" code
which does this kind of things as long as it's done in a generic enought
way that you don't have to tweak it often.
If the extra indirections don't cost anything (except extra code) then
maybe it's fine keeping. I would say the main driver to clean such code
would be if you feel others need to hack on it and it need to be cleaner
and easier to understand.
Daniel
The generator is in a state where adding new stuff from the vSphere
API boils down to just adding a definition to the input file in most
cases. For some things list in the generator need to be tweaked that
define what features should be generated for a specific object type.
IIRC in most cases this could be automated too as the generator could
detect this by analyzing the input file better. This way the generator
becomes more complete and others don't need to hack on it and the code
generation process to add new stuff from the vSphere API.
About the complexity in the generated code: For example parts of the
type handling currently nest three levels of macros, because it's
"convenient" and reduces manual code duplication, but it becomes
harder to understand and get right when I need to hack on it. That's
the motivation behind possibly simplifying the generator related stuff
:)
Matthias
4958
days inactive
4960
days old
5 comments
2 participants
participants (2)
-
Daniel Veillard -
Matthias Bolte