Re: [PATCH 2/2] mscoree: There can be only one QI implementation i a COM object!
These look like 2 separate COM objects to me. They have different IUnknown pointers and a different set of implemented interfaces. Is there a reason to link them? On Thu, Sep 8, 2016 at 2:25 PM, Michael Stefaniuc <mstefani(a)redhat.de> wrote:
Signed-off-by: Michael Stefaniuc <mstefani(a)redhat.de> --- dlls/mscoree/config.c | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-)
diff --git a/dlls/mscoree/config.c b/dlls/mscoree/config.c index b8bf882..a52a521 100644 --- a/dlls/mscoree/config.c +++ b/dlls/mscoree/config.c @@ -73,18 +73,19 @@ static inline ConfigFileHandler *impl_from_ISAXErrorHandler(ISAXErrorHandler *if static HRESULT WINAPI ConfigFileHandler_QueryInterface(ISAXContentHandler *iface, REFIID riid, void **ppvObject) { - if (IsEqualGUID(riid, &IID_ISAXContentHandler) || - IsEqualGUID(riid, &IID_IUnknown)) - { - *ppvObject = iface; - } + ConfigFileHandler *This = impl_from_ISAXContentHandler(iface); + + if (IsEqualGUID(riid, &IID_ISAXContentHandler) || IsEqualGUID(riid, &IID_IUnknown)) + *ppvObject = &This->ISAXContentHandler_iface; + else if (IsEqualGUID(riid, &IID_ISAXErrorHandler)) + *ppvObject = &This->ISAXErrorHandler_iface; else { WARN("Unsupported interface %s\n", debugstr_guid(riid)); return E_NOINTERFACE; }
- ISAXContentHandler_AddRef(iface); + IUnknown_AddRef((IUnknown*)*ppvObject);
return S_OK; } @@ -377,20 +378,8 @@ static const struct ISAXContentHandlerVtbl ConfigFileHandlerVtbl = static HRESULT WINAPI ConfigFileHandler_Error_QueryInterface(ISAXErrorHandler *iface, REFIID riid, void **ppvObject) { - if (IsEqualGUID(riid, &IID_ISAXErrorHandler) || - IsEqualGUID(riid, &IID_IUnknown)) - { - *ppvObject = iface; - } - else - { - WARN("Unsupported interface %s\n", debugstr_guid(riid)); - return E_NOINTERFACE; - } - - ISAXErrorHandler_AddRef(iface); - - return S_OK; + ConfigFileHandler *This = impl_from_ISAXErrorHandler(iface); + return ISAXContentHandler_QueryInterface(&This->ISAXContentHandler_iface, riid, ppvObject); }
static ULONG WINAPI ConfigFileHandler_Error_AddRef(ISAXErrorHandler *iface) -- 2.7.4
On 09/09/2016 02:45 AM, Vincent Povirk wrote:
These look like 2 separate COM objects to me. They have different IUnknown pointers and a different set of implemented interfaces. Is there a reason to link them? Uhm... those are two different interfaces but they were already implemented in the same COM object aka struct ConfigFileHandler. See my first patch were the AddRef and Release methods were already forwarded from ISAXErrorHandler to ISAXContentHandler. So I just cleaned up COM for the single COM object.
The alternative is to split the struct ConfigFileHandler. But as this is not part of the API/ABI and the applications cannot get to those COM interfaces it doesn't matters. Also the ISAXXMLReader just needs the two COM interfaces, how those are implemented it doesn't matters either. bye michael
On Thu, Sep 8, 2016 at 2:25 PM, Michael Stefaniuc <mstefani(a)redhat.de> wrote:
Signed-off-by: Michael Stefaniuc <mstefani(a)redhat.de> --- dlls/mscoree/config.c | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-)
diff --git a/dlls/mscoree/config.c b/dlls/mscoree/config.c index b8bf882..a52a521 100644 --- a/dlls/mscoree/config.c +++ b/dlls/mscoree/config.c @@ -73,18 +73,19 @@ static inline ConfigFileHandler *impl_from_ISAXErrorHandler(ISAXErrorHandler *if static HRESULT WINAPI ConfigFileHandler_QueryInterface(ISAXContentHandler *iface, REFIID riid, void **ppvObject) { - if (IsEqualGUID(riid, &IID_ISAXContentHandler) || - IsEqualGUID(riid, &IID_IUnknown)) - { - *ppvObject = iface; - } + ConfigFileHandler *This = impl_from_ISAXContentHandler(iface); + + if (IsEqualGUID(riid, &IID_ISAXContentHandler) || IsEqualGUID(riid, &IID_IUnknown)) + *ppvObject = &This->ISAXContentHandler_iface; + else if (IsEqualGUID(riid, &IID_ISAXErrorHandler)) + *ppvObject = &This->ISAXErrorHandler_iface; else { WARN("Unsupported interface %s\n", debugstr_guid(riid)); return E_NOINTERFACE; }
- ISAXContentHandler_AddRef(iface); + IUnknown_AddRef((IUnknown*)*ppvObject);
return S_OK; } @@ -377,20 +378,8 @@ static const struct ISAXContentHandlerVtbl ConfigFileHandlerVtbl = static HRESULT WINAPI ConfigFileHandler_Error_QueryInterface(ISAXErrorHandler *iface, REFIID riid, void **ppvObject) { - if (IsEqualGUID(riid, &IID_ISAXErrorHandler) || - IsEqualGUID(riid, &IID_IUnknown)) - { - *ppvObject = iface; - } - else - { - WARN("Unsupported interface %s\n", debugstr_guid(riid)); - return E_NOINTERFACE; - } - - ISAXErrorHandler_AddRef(iface); - - return S_OK; + ConfigFileHandler *This = impl_from_ISAXErrorHandler(iface); + return ISAXContentHandler_QueryInterface(&This->ISAXContentHandler_iface, riid, ppvObject); }
static ULONG WINAPI ConfigFileHandler_Error_AddRef(ISAXErrorHandler *iface) -- 2.7.4
-- Michael Stefaniuc Tel.: +49-711-96437-199 Manager, APAC/EMEA IT Network Fax.: +49-711-96437-111 -------------------------------------------------------------------- Reg. Adresse: Red Hat GmbH, Werner-von-Siemens-Ring 14, 85630 Grasbrunn Handelsregister: Amtsgericht Muenchen HRB 153243 Geschäftsführer: Charles Cachera, Michael Cunningham, Michael O'Neill, Eric Shander
Uhm... those are two different interfaces but they were already implemented in the same COM object aka struct ConfigFileHandler. See my first patch were the AddRef and Release methods were already forwarded from ISAXErrorHandler to ISAXContentHandler. So I just cleaned up COM for the single COM object.
A COM object is not the same as a struct. A single struct can implement multiple COM objects (we do this for many interfaces in windowscodecs where we have a "container" object that always contains at least one child), or a single COM object can be implemented via multiple structs. The identity of a COM object is determined by the result of calling QueryInterface with IID_IUnknown. In this case, QueryInterface returns different IUnknown pointers for each interface in the struct, so they are different objects. This imposes some requirements on their behavior, but there's no requirement that the refcounts of two different objects be unrelated, or that the reference counting be implemented in any specific way. There's no reason they have to be different objects here, but there are many other structs in windowscodecs that implement multiple COM objects which must be separate.
The alternative is to split the struct ConfigFileHandler. But as this is not part of the API/ABI and the applications cannot get to those COM interfaces it doesn't matters. Also the ISAXXMLReader just needs the two COM interfaces, how those are implemented it doesn't matters either.
There's no requirement that a single struct can only implement one COM object. The implementation of most encoders and decoders in windowscodecs is greatly simplified by putting multiple objects in one struct.
On 09/09/2016 04:25 PM, Vincent Povirk wrote:
Uhm... those are two different interfaces but they were already implemented in the same COM object aka struct ConfigFileHandler. See my first patch were the AddRef and Release methods were already forwarded from ISAXErrorHandler to ISAXContentHandler. So I just cleaned up COM for the single COM object.
A COM object is not the same as a struct. A single struct can implement multiple COM objects (we do this for many interfaces in windowscodecs where we have a "container" object that always contains First time I hit that. All the others were COM violations.
at least one child), or a single COM object can be implemented via multiple structs. I have merged quite a few of those during the COM cleanup as there is no advantage in splitting up the IFoo_iface field over multiple structs but I have seen different downsides (complex/more code, refcounting done wrong, memory leaks).
The identity of a COM object is determined by the result of calling QueryInterface with IID_IUnknown. In this case, QueryInterface returns different IUnknown pointers for each interface in the struct, so they Then you need to comment that as this is the exception of how COM is implemented in the rest of Wine. And more importantly it is indistinguishable from a COM violation bug,
are different objects. This imposes some requirements on their behavior, but there's no requirement that the refcounts of two different objects be unrelated, or that the reference counting be implemented in any specific way. In this particular case you are right. But in general the refcount is visible to applications. while(IFoo_Release(iface)); is a common pattern.
There's no reason they have to be different objects here, but there That's my point. Principle of the least surprise, if you don't need to use a non standard construct then don't do it.
are many other structs in windowscodecs that implement multiple COM objects which must be separate.
The alternative is to split the struct ConfigFileHandler. But as this is not part of the API/ABI and the applications cannot get to those COM interfaces it doesn't matters. Also the ISAXXMLReader just needs the two COM interfaces, how those are implemented it doesn't matters either.
There's no requirement that a single struct can only implement one COM object. The implementation of most encoders and decoders in windowscodecs is greatly simplified by putting multiple objects in one struct. Then just use a union for all those interface fields. That makes it obvious on what is going on. But afair there are other patterns used throughout Wine that deal with a similar problem.
bye michael
participants (2)
-
Michael Stefaniuc -
Vincent Povirk