Re: [libvirt] [PATCH v7 3/6] hyperv: add hypervInvokeMethod
[Please don't top-post on technical mailing lists]
2017-07-19 14:13 GMT+02:00 Sri Ramanujam <sramanujam(a)datto.com>:
Just tested this patch quickly, and it causes invalid free()s when
the
codepath is invoked against a Hyper-V 2008 system, and causes the
operation to fail (but does not crash virsh) against Hyper-V 2012. I'm
away from my usual setup atm (on vacation), so I can help look into
this further next week when I'm back.
On Wed, Jul 19, 2017 at 1:21 AM, Matthias Bolte
<matthias.bolte(a)googlemail.com> wrote:
> 2017-07-18 18:54 GMT+02:00 Andrea Bolognani <abologna(a)redhat.com>:
>> On Tue, 2017-06-27 at 15:13 -0400, Sri Ramanujam wrote:
>>> This commit adds support for invoking methods on remote objects
>>> via hypervInvokeMethod.
>>> ---
>>> src/hyperv/hyperv_wmi.c | 590
++++++++++++++++++++++++++++++++++++++++++++++++
>>> src/hyperv/hyperv_wmi.h | 8 +-
>>> src/hyperv/openwsman.h | 4 +
>>> 3 files changed, 600 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c
>>> index 2732db3..f944b14 100644
>>> --- a/src/hyperv/hyperv_wmi.c
>>> +++ b/src/hyperv/hyperv_wmi.c
>> [...]
>>> +static int
>>> +hypervSerializeEprParam(hypervParamPtr p, hypervPrivate *priv,
>>> + const char *resourceUri, WsXmlDocH doc, WsXmlNodeH *methodNode)
>>> +{
>>> + int result = -1;
>>> + WsXmlNodeH xmlNodeParam = NULL,
>>> + xmlNodeTemp = NULL,
>>> + xmlNodeAddr = NULL,
>>> + xmlNodeRef = NULL;
>>> + xmlNodePtr xmlNodeAddrPtr = NULL,
>>> + xmlNodeRefPtr = NULL;
>> [...]
>>> + if (!(xmlNodeAddrPtr = xmlDocCopyNode((xmlNodePtr) xmlNodeAddr, docPtr,
1))) {
>>
>> Here you're casting a WsXmlNodeH to a xmlNodePtr, and clang
>> doesn't like it one bit:
>>
>> hyperv/hyperv_wmi.c:576:43: error:
>> cast from 'WsXmlNodeH' (aka 'struct __WsXmlNode *')
>> to 'xmlNodePtr' (aka 'struct _xmlNode *')
>> increases required alignment from 4 to 8
>> [-Werror,-Wcast-align]
>>
>> Any idea how to unbreak it?
>
> The problem here is that the driver is mixing direct libxml2 calls
> with calls to the libxml2 wrapper of openwsman. The openwsman wrapper
> type WsXmlNodeH is actually a xmlNodePtr, but that is hidden to the
> compiler.
>
> I checked if the openwsman libxml2 wrapper is complete enough to get
> rid of this API mixing. I could replace all direct libxml2 calls with
> openwsman wrapper call except xmlNewCDataBlock. A hack for this last
> offender is to cast to a void pointer first, instead of a direct cast.
> See attached patch for a quick fix, compile-tested only.
>
> Another possibility is to do all the XML building using direct libxml2
> calls, format the XML document and reparse it with the openwsman
> wrapper. But I don't have time to work on that at the moment.
>
The patch was not meant to fix the issue, it was meant to illustrate
what I tried to investigate a potential way to fix the problem.
I think, there are three ways to go about this:
1. Just cast to a void pointer first. This silences clang, but still
relies on the implementation detail that openwsman is using libxml2
internally. This is the quick and dirty workaround for the immediate
problem at hand.
2. Clean up the API mix and only use the public ws_xml_* functions.
That will probably not work because the openwsman wrapper doesn't
provide a function to create a CDATA block.
3. Clean up the API mix and only use libxml2 functions to build the
XML and then format and reparse it to obtain an WsXmlDocH.
Regards,
Matthias
--
Matthias Bolte
http://photron.blogspot.com