1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed

Chen Hanxiao posted 1 patch 6 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170929091122.22726-1-chen_han_xiao@126.com
Test checkpatch passed
Test docker passed
Test s390x passed
qga/vss-win32/install.cpp | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
[Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
Posted by Chen Hanxiao 6 years, 6 months ago 
From: Chen Hanxiao <chenhanxiao@gmail.com>

On some of windows (win08 sp2),
it doesn't work by calling LookupAccountSidW with
well-known SIDs,
We got an error:
    error 997 overlapped I/O operation is in progress

But hardcoded names work.

This patch introduces a workaroud for this issue:
if LookupAccountSidW failed, try hardcoded one.

Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com>
---
 qga/vss-win32/install.cpp | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
index ba7c94eb25..dcf6299af9 100644
--- a/qga/vss-win32/install.cpp
+++ b/qga/vss-win32/install.cpp
@@ -312,7 +312,10 @@ STDAPI COMRegister(void)
 
     /* Setup roles of the applicaion */
 
-    chk(getNameByStringSID(administratorsGroupSID, buffer, &bufferLen));
+    hr = getNameByStringSID(administratorsGroupSID, buffer, &bufferLen);
+    if (FAILED(hr)) {
+        wsprintfW(buffer, L"%ls", L"Administrators");
+    }
     chk(pApps->GetCollection(_bstr_t(L"Roles"), key,
                              (IDispatch **)pRoles.replace()));
     chk(pRoles->Populate());
@@ -333,7 +336,10 @@ STDAPI COMRegister(void)
     chk(put_Value(pObj, L"User", _bstr_t(".\\") + name));
 
     bufferLen = BUFFER_SIZE;
-    chk(getNameByStringSID(systemUserSID, buffer, &bufferLen));
+    hr = getNameByStringSID(systemUserSID, buffer, &bufferLen);
+    if (FAILED(hr)) {
+        wsprintfW(buffer, L"%ls", L"SYSTEM");
+    }
     chk(pUsersInRole->Add((IDispatch **)pObj.replace()));
     chk(put_Value(pObj, L"User", buffer));
     chk(pUsersInRole->SaveChanges(&n));
-- 
2.13.5
Re: [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
Posted by Tomáš Golembiovský 6 years, 6 months ago 
On Fri, 29 Sep 2017 17:11:22 +0800
Chen Hanxiao <chen_han_xiao@126.com> wrote:

> From: Chen Hanxiao <chenhanxiao@gmail.com>
> 
> On some of windows (win08 sp2),
> it doesn't work by calling LookupAccountSidW with
> well-known SIDs,
> We got an error:
>     error 997 overlapped I/O operation is in progress
> 
> But hardcoded names work.
> 
> This patch introduces a workaroud for this issue:
> if LookupAccountSidW failed, try hardcoded one.
> 
> Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com>
> ---
>  qga/vss-win32/install.cpp | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 

I wonder if it's caused by qga itself or a race/conflict with some other
app. But still...


Reviewed-by: Tomáš Golembiovský <tgolembi@redhat.com>


-- 
Tomáš Golembiovský <tgolembi@redhat.com>
Re: [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
Posted by Michael Roth 6 years, 6 months ago 
Quoting Tomáš Golembiovský (2017-09-29 16:31:02)
> On Fri, 29 Sep 2017 17:11:22 +0800
> Chen Hanxiao <chen_han_xiao@126.com> wrote:
> 
> > From: Chen Hanxiao <chenhanxiao@gmail.com>
> > 
> > On some of windows (win08 sp2),
> > it doesn't work by calling LookupAccountSidW with
> > well-known SIDs,
> > We got an error:
> >     error 997 overlapped I/O operation is in progress
> > 
> > But hardcoded names work.
> > 
> > This patch introduces a workaroud for this issue:
> > if LookupAccountSidW failed, try hardcoded one.
> > 
> > Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com>
> > ---
> >  qga/vss-win32/install.cpp | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> 
> I wonder if it's caused by qga itself or a race/conflict with some other
> app. But still...
> 
> 
> Reviewed-by: Tomáš Golembiovský <tgolembi@redhat.com>

I think it might be getNameByStringSID()/LookupAccountSidW() reporting a
stale GetLastError() value.

Does this fix the issue?

diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
index ba7c94eb25..94c921e8de 100644
--- a/qga/vss-win32/install.cpp
+++ b/qga/vss-win32/install.cpp
@@ -149,9 +149,10 @@ static HRESULT getNameByStringSID(
     wchar_t domainName[BUFFER_SIZE];
 
     chk(ConvertStringSidToSidW(sid, &psid));
-    LookupAccountSidW(NULL, psid, buffer, bufferLen,
-                domainName, &domainNameLen, &groupType);
-    hr = HRESULT_FROM_WIN32(GetLastError());
+    if (!LookupAccountSidW(NULL, psid, buffer, bufferLen,
+                           domainName, &domainNameLen, &groupType)) {
+        hr = HRESULT_FROM_WIN32(GetLastError());
+    }
 
     LocalFree(psid);

> 
> 
> -- 
> Tomáš Golembiovský <tgolembi@redhat.com>
>
Re: [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
Posted by Chen Hanxiao 6 years, 6 months ago 

At 2017-10-26 05:58:07, "Michael Roth" <mdroth@linux.vnet.ibm.com> wrote:
>Quoting Tomáš Golembiovský (2017-09-29 16:31:02)
>> On Fri, 29 Sep 2017 17:11:22 +0800
>> Chen Hanxiao <chen_han_xiao@126.com> wrote:
>> 
>> > From: Chen Hanxiao <chenhanxiao@gmail.com>
>> > 
>> > On some of windows (win08 sp2),
>> > it doesn't work by calling LookupAccountSidW with
>> > well-known SIDs,
>> > We got an error:
>> >     error 997 overlapped I/O operation is in progress
>> > 
>> > But hardcoded names work.
>> > 
>> > This patch introduces a workaroud for this issue:
>> > if LookupAccountSidW failed, try hardcoded one.
>> > 
>> > Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com>
>> > ---
>> >  qga/vss-win32/install.cpp | 10 ++++++++--
>> >  1 file changed, 8 insertions(+), 2 deletions(-)
>> > 
>> 
>> I wonder if it's caused by qga itself or a race/conflict with some other
>> app. But still...
>> 
>> 
>> Reviewed-by: Tomáš Golembiovský <tgolembi@redhat.com>
>
>I think it might be getNameByStringSID()/LookupAccountSidW() reporting a
>stale GetLastError() value.
>
>Does this fix the issue?

Not exactly.
I tested your patch several times, it improved greatly.
But failed only one time,
got another error 1722.

Build with my fallback patch and your suggestion, 
installation work perfectly.

Regards,
- Chen

>
>diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
>index ba7c94eb25..94c921e8de 100644
>--- a/qga/vss-win32/install.cpp
>+++ b/qga/vss-win32/install.cpp
>@@ -149,9 +149,10 @@ static HRESULT getNameByStringSID(
>     wchar_t domainName[BUFFER_SIZE];
> 
>     chk(ConvertStringSidToSidW(sid, &psid));
>-    LookupAccountSidW(NULL, psid, buffer, bufferLen,
>-                domainName, &domainNameLen, &groupType);
>-    hr = HRESULT_FROM_WIN32(GetLastError());
>+    if (!LookupAccountSidW(NULL, psid, buffer, bufferLen,
>+                           domainName, &domainNameLen, &groupType)) {
>+        hr = HRESULT_FROM_WIN32(GetLastError());
>+    }
> 
>     LocalFree(psid);
>
>> 
>> 
>> -- 
>> Tomáš Golembiovský <tgolembi@redhat.com>
>> 
>
Re: [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
Posted by Michael Roth 6 years, 6 months ago 
Quoting Chen Hanxiao (2017-10-26 04:27:40)
> 
> 
> At 2017-10-26 05:58:07, "Michael Roth" <mdroth@linux.vnet.ibm.com> wrote:
> >Quoting Tomáš Golembiovský (2017-09-29 16:31:02)
> >> On Fri, 29 Sep 2017 17:11:22 +0800
> >> Chen Hanxiao <chen_han_xiao@126.com> wrote:
> >> 
> >> > From: Chen Hanxiao <chenhanxiao@gmail.com>
> >> > 
> >> > On some of windows (win08 sp2),
> >> > it doesn't work by calling LookupAccountSidW with
> >> > well-known SIDs,
> >> > We got an error:
> >> >     error 997 overlapped I/O operation is in progress
> >> > 
> >> > But hardcoded names work.
> >> > 
> >> > This patch introduces a workaroud for this issue:
> >> > if LookupAccountSidW failed, try hardcoded one.
> >> > 
> >> > Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com>
> >> > ---
> >> >  qga/vss-win32/install.cpp | 10 ++++++++--
> >> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >> > 
> >> 
> >> I wonder if it's caused by qga itself or a race/conflict with some other
> >> app. But still...
> >> 
> >> 
> >> Reviewed-by: Tomáš Golembiovský <tgolembi@redhat.com>
> >
> >I think it might be getNameByStringSID()/LookupAccountSidW() reporting a
> >stale GetLastError() value.
> >
> >Does this fix the issue?
> 
> Not exactly.
> I tested your patch several times, it improved greatly.
> But failed only one time,
> got another error 1722.

Hmm, was that error also from the getNameByStringSID() call?

> 
> Build with my fallback patch and your suggestion, 
> installation work perfectly.

Thanks for testing.

> 
> Regards,
> - Chen
> 
> >
> >diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
> >index ba7c94eb25..94c921e8de 100644
> >--- a/qga/vss-win32/install.cpp
> >+++ b/qga/vss-win32/install.cpp
> >@@ -149,9 +149,10 @@ static HRESULT getNameByStringSID(
> >     wchar_t domainName[BUFFER_SIZE];
> > 
> >     chk(ConvertStringSidToSidW(sid, &psid));
> >-    LookupAccountSidW(NULL, psid, buffer, bufferLen,
> >-                domainName, &domainNameLen, &groupType);
> >-    hr = HRESULT_FROM_WIN32(GetLastError());
> >+    if (!LookupAccountSidW(NULL, psid, buffer, bufferLen,
> >+                           domainName, &domainNameLen, &groupType)) {
> >+        hr = HRESULT_FROM_WIN32(GetLastError());
> >+    }
> > 
> >     LocalFree(psid);
> >
> >> 
> >> 
> >> -- 
> >> Tomáš Golembiovský <tgolembi@redhat.com>
> >> 
> >
Re: [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
Posted by Chen Hanxiao 6 years, 6 months ago 
At 2017-10-26 17:59:34, "Michael Roth" <mdroth@linux.vnet.ibm.com> wrote:
>Quoting Chen Hanxiao (2017-10-26 04:27:40)
>> 
>> 
>> At 2017-10-26 05:58:07, "Michael Roth" <mdroth@linux.vnet.ibm.com> wrote:
>> >Quoting Tomáš Golembiovský (2017-09-29 16:31:02)
>> >> On Fri, 29 Sep 2017 17:11:22 +0800
>> >> Chen Hanxiao <chen_han_xiao@126.com> wrote:
>> >> 
>> >> > From: Chen Hanxiao <chenhanxiao@gmail.com>
>> >> > 
>> >> > On some of windows (win08 sp2),
>> >> > it doesn't work by calling LookupAccountSidW with
>> >> > well-known SIDs,
>> >> > We got an error:
>> >> >     error 997 overlapped I/O operation is in progress
>> >> > 
>> >> > But hardcoded names work.
>> >> > 
>> >> > This patch introduces a workaroud for this issue:
>> >> > if LookupAccountSidW failed, try hardcoded one.
>> >> > 
>> >> > Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com>
>> >> > ---
>> >> >  qga/vss-win32/install.cpp | 10 ++++++++--
>> >> >  1 file changed, 8 insertions(+), 2 deletions(-)
>> >> > 
>> >> 
>> >> I wonder if it's caused by qga itself or a race/conflict with some other
>> >> app. But still...
>> >> 
>> >> 
>> >> Reviewed-by: Tomáš Golembiovský <tgolembi@redhat.com>
>> >
>> >I think it might be getNameByStringSID()/LookupAccountSidW() reporting a
>> >stale GetLastError() value.
>> >
>> >Does this fix the issue?
>> 
>> Not exactly.
>> I tested your patch several times, it improved greatly.
>> But failed only one time,
>> got another error 1722.
>
>Hmm, was that error also from the getNameByStringSID() call?

I don't know how to trace qemu-ga-win,
but added some fprintf(stdout).

+    hr = getNameByStringSID(administratorsGroupSID, buffer, &bufferLen);
+    if (FAILED(hr)) {

I saw my logs inside if (FAILED(hr)) {

But it looks weird, as your patch already dealing with this senario.


Regards,
- Chen

>
>> 
>> Build with my fallback patch and your suggestion, 
>> installation work perfectly.
>
>Thanks for testing.
>
>>
Re: [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
Posted by Tomáš Golembiovský 6 years, 6 months ago 
On Wed, 25 Oct 2017 16:58:07 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> Quoting Tomáš Golembiovský (2017-09-29 16:31:02)
> > On Fri, 29 Sep 2017 17:11:22 +0800
> > Chen Hanxiao <chen_han_xiao@126.com> wrote:
> >   
> > > From: Chen Hanxiao <chenhanxiao@gmail.com>
> > > 
> > > On some of windows (win08 sp2),
> > > it doesn't work by calling LookupAccountSidW with
> > > well-known SIDs,
> > > We got an error:
> > >     error 997 overlapped I/O operation is in progress
> > > 
> > > But hardcoded names work.
> > > 
> > > This patch introduces a workaroud for this issue:
> > > if LookupAccountSidW failed, try hardcoded one.
> > > 
> > > Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com>
> > > ---
> > >  qga/vss-win32/install.cpp | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > >   
> > 
> > I wonder if it's caused by qga itself or a race/conflict with some other
> > app. But still...
> > 
> > 
> > Reviewed-by: Tomáš Golembiovský <tgolembi@redhat.com>  
> 
> I think it might be getNameByStringSID()/LookupAccountSidW() reporting a
> stale GetLastError() value.
> 

I suppose you're right! Taking a closer look there's one more issue with
getNameByStringSID(). The call ConvertStringSidToSidW() does not return
HRESULT so using chk() makes no sense.

I propose slightly modified version of your fix:

diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
index ba7c94eb25..65f68f94a3 100644
--- a/qga/vss-win32/install.cpp
+++ b/qga/vss-win32/install.cpp
@@ -148,10 +148,15 @@ static HRESULT getNameByStringSID(
     DWORD domainNameLen = BUFFER_SIZE;
     wchar_t domainName[BUFFER_SIZE];
 
-    chk(ConvertStringSidToSidW(sid, &psid));
-    LookupAccountSidW(NULL, psid, buffer, bufferLen,
-                domainName, &domainNameLen, &groupType);
-    hr = HRESULT_FROM_WIN32(GetLastError());
+    if (!ConvertStringSidToSidW(sid, &psid) {
+        hr = HRESULT_FROM_WIN32(GetLastError());
+        goto out;
+    }
+    if (!LookupAccountSidW(NULL, psid, buffer, bufferLen,
+                           domainName, &domainNameLen, &groupType)) {
+        hr = HRESULT_FROM_WIN32(GetLastError());
+        /* Fall through and free psid */
+    }
 
     LocalFree(psid);
 

-- 
Tomáš Golembiovský <tgolembi@redhat.com>
Re: [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
Posted by Michael Roth 6 years, 6 months ago 
Quoting Tomáš Golembiovský (2017-10-26 08:54:37)
> On Wed, 25 Oct 2017 16:58:07 -0500
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > Quoting Tomáš Golembiovský (2017-09-29 16:31:02)
> > > On Fri, 29 Sep 2017 17:11:22 +0800
> > > Chen Hanxiao <chen_han_xiao@126.com> wrote:
> > >   
> > > > From: Chen Hanxiao <chenhanxiao@gmail.com>
> > > > 
> > > > On some of windows (win08 sp2),
> > > > it doesn't work by calling LookupAccountSidW with
> > > > well-known SIDs,
> > > > We got an error:
> > > >     error 997 overlapped I/O operation is in progress
> > > > 
> > > > But hardcoded names work.
> > > > 
> > > > This patch introduces a workaroud for this issue:
> > > > if LookupAccountSidW failed, try hardcoded one.
> > > > 
> > > > Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com>
> > > > ---
> > > >  qga/vss-win32/install.cpp | 10 ++++++++--
> > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > >   
> > > 
> > > I wonder if it's caused by qga itself or a race/conflict with some other
> > > app. But still...
> > > 
> > > 
> > > Reviewed-by: Tomáš Golembiovský <tgolembi@redhat.com>  
> > 
> > I think it might be getNameByStringSID()/LookupAccountSidW() reporting a
> > stale GetLastError() value.
> > 
> 
> I suppose you're right! Taking a closer look there's one more issue with
> getNameByStringSID(). The call ConvertStringSidToSidW() does not return
> HRESULT so using chk() makes no sense.
> 
> I propose slightly modified version of your fix:
> 
> diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
> index ba7c94eb25..65f68f94a3 100644
> --- a/qga/vss-win32/install.cpp
> +++ b/qga/vss-win32/install.cpp
> @@ -148,10 +148,15 @@ static HRESULT getNameByStringSID(
>      DWORD domainNameLen = BUFFER_SIZE;
>      wchar_t domainName[BUFFER_SIZE];
>  
> -    chk(ConvertStringSidToSidW(sid, &psid));
> -    LookupAccountSidW(NULL, psid, buffer, bufferLen,
> -                domainName, &domainNameLen, &groupType);
> -    hr = HRESULT_FROM_WIN32(GetLastError());
> +    if (!ConvertStringSidToSidW(sid, &psid) {
> +        hr = HRESULT_FROM_WIN32(GetLastError());
> +        goto out;
> +    }
> +    if (!LookupAccountSidW(NULL, psid, buffer, bufferLen,
> +                           domainName, &domainNameLen, &groupType)) {
> +        hr = HRESULT_FROM_WIN32(GetLastError());
> +        /* Fall through and free psid */
> +    }
>  
>      LocalFree(psid);


Thanks! It's not yet clear if this fixes the issue completely (though it
seems likely), but it's clearly a bug either way so I've gone ahead and
applied it to the qga tree:

  https://github.com/mdroth/qemu/commit/8cedc80555e5a395a4fda7130b0115015e775c48

Chen, if there's still an issue with the updated patch please let me know.

>  
> 
> -- 
> Tomáš Golembiovský <tgolembi@redhat.com>
>
Re: [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
Posted by Chen Hanxiao 6 years, 5 months ago 
At 2017-10-27 09:05:25, "Michael Roth" <mdroth@linux.vnet.ibm.com> wrote:
>Quoting Tomáš Golembiovský (2017-10-26 08:54:37)
>> On Wed, 25 Oct 2017 16:58:07 -0500
>> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
>> 
>> > Quoting Tomáš Golembiovský (2017-09-29 16:31:02)
>> > > On Fri, 29 Sep 2017 17:11:22 +0800
>> > > Chen Hanxiao <chen_han_xiao@126.com> wrote:
>> > > 
>> > > Reviewed-by: Tomáš Golembiovský <tgolembi@redhat.com>  
>> > 
>> > I think it might be getNameByStringSID()/LookupAccountSidW() reporting a
>> > stale GetLastError() value.
>> > 
>> 
>> I suppose you're right! Taking a closer look there's one more issue with
>> getNameByStringSID(). The call ConvertStringSidToSidW() does not return
>> HRESULT so using chk() makes no sense.
>> 
>> I propose slightly modified version of your fix:
>> 
>> diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
>> index ba7c94eb25..65f68f94a3 100644
>> --- a/qga/vss-win32/install.cpp
>> +++ b/qga/vss-win32/install.cpp
>> @@ -148,10 +148,15 @@ static HRESULT getNameByStringSID(
>>      DWORD domainNameLen = BUFFER_SIZE;
>>      wchar_t domainName[BUFFER_SIZE];
>>  
>> -    chk(ConvertStringSidToSidW(sid, &psid));
>> -    LookupAccountSidW(NULL, psid, buffer, bufferLen,
>> -                domainName, &domainNameLen, &groupType);
>> -    hr = HRESULT_FROM_WIN32(GetLastError());
>> +    if (!ConvertStringSidToSidW(sid, &psid) {
>> +        hr = HRESULT_FROM_WIN32(GetLastError());
>> +        goto out;
>> +    }
>> +    if (!LookupAccountSidW(NULL, psid, buffer, bufferLen,
>> +                           domainName, &domainNameLen, &groupType)) {
>> +        hr = HRESULT_FROM_WIN32(GetLastError());
>> +        /* Fall through and free psid */
>> +    }
>>  
>>      LocalFree(psid);
>
>
>Thanks! It's not yet clear if this fixes the issue completely (though it
>seems likely), but it's clearly a bug either way so I've gone ahead and
>applied it to the qga tree:
>
>  https://github.com/mdroth/qemu/commit/8cedc80555e5a395a4fda7130b0115015e775c48
>
>Chen, if there's still an issue with the updated patch please let me know.

Sorry for the late reply.

I tested the upstream qga-win on that windows VM for several times.
It works fine by my limit tests.

Regards,
- Chen



>
>>  
>> 
>> -- 
>> Tomáš Golembiovský <tgolembi@redhat.com>
>> 
>
Re: [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
Posted by Tomáš Golembiovský 6 years, 5 months ago 
On Thu, 2 Nov 2017 18:58:49 +0800 (CST)
"Chen Hanxiao" <chen_han_xiao@126.com> wrote:

> At 2017-10-27 09:05:25, "Michael Roth" <mdroth@linux.vnet.ibm.com> wrote:
> >Quoting Tomáš Golembiovský (2017-10-26 08:54:37)  
> >> On Wed, 25 Oct 2017 16:58:07 -0500
> >> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> >>   
> >> > Quoting Tomáš Golembiovský (2017-09-29 16:31:02)  
> >> > > On Fri, 29 Sep 2017 17:11:22 +0800
> >> > > Chen Hanxiao <chen_han_xiao@126.com> wrote:
> >> > > 
> >> > > Reviewed-by: Tomáš Golembiovský <tgolembi@redhat.com>    
> >> > 
> >> > I think it might be getNameByStringSID()/LookupAccountSidW() reporting a
> >> > stale GetLastError() value.
> >> >   
> >> 
> >> I suppose you're right! Taking a closer look there's one more issue with
> >> getNameByStringSID(). The call ConvertStringSidToSidW() does not return
> >> HRESULT so using chk() makes no sense.
> >> 
> >> I propose slightly modified version of your fix:
> >> 
> >> diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
> >> index ba7c94eb25..65f68f94a3 100644
> >> --- a/qga/vss-win32/install.cpp
> >> +++ b/qga/vss-win32/install.cpp
> >> @@ -148,10 +148,15 @@ static HRESULT getNameByStringSID(
> >>      DWORD domainNameLen = BUFFER_SIZE;
> >>      wchar_t domainName[BUFFER_SIZE];
> >>  
> >> -    chk(ConvertStringSidToSidW(sid, &psid));
> >> -    LookupAccountSidW(NULL, psid, buffer, bufferLen,
> >> -                domainName, &domainNameLen, &groupType);
> >> -    hr = HRESULT_FROM_WIN32(GetLastError());
> >> +    if (!ConvertStringSidToSidW(sid, &psid) {
> >> +        hr = HRESULT_FROM_WIN32(GetLastError());
> >> +        goto out;
> >> +    }
> >> +    if (!LookupAccountSidW(NULL, psid, buffer, bufferLen,
> >> +                           domainName, &domainNameLen, &groupType)) {
> >> +        hr = HRESULT_FROM_WIN32(GetLastError());
> >> +        /* Fall through and free psid */
> >> +    }
> >>  
> >>      LocalFree(psid);  
> >
> >
> >Thanks! It's not yet clear if this fixes the issue completely (though it
> >seems likely), but it's clearly a bug either way so I've gone ahead and
> >applied it to the qga tree:
> >
> >  https://github.com/mdroth/qemu/commit/8cedc80555e5a395a4fda7130b0115015e775c48
> >
> >Chen, if there's still an issue with the updated patch please let me know.  
> 
> Sorry for the late reply.
> 
> I tested the upstream qga-win on that windows VM for several times.
> It works fine by my limit tests.
> 

I'm glad to hear that.

    Tomas

> Regards,
> - Chen
> 
> 
> 
> >  
> >>  
> >> 
> >> -- 
> >> Tomáš Golembiovský <tgolembi@redhat.com>
> >>   
> >  


-- 
Tomáš Golembiovský <tgolembi@redhat.com>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.