* [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
@ 2017-09-29 9:11 Chen Hanxiao
2017-09-29 21:31 ` Tomáš Golembiovský
0 siblings, 1 reply; 10+ messages in thread
From: Chen Hanxiao @ 2017-09-29 9:11 UTC (permalink / raw)
To: mdroth; +Cc: qemu-devel, Chen Hanxiao
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
2017-09-29 9:11 [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed Chen Hanxiao
@ 2017-09-29 21:31 ` Tomáš Golembiovský
2017-10-25 21:58 ` Michael Roth
0 siblings, 1 reply; 10+ messages in thread
From: Tomáš Golembiovský @ 2017-09-29 21:31 UTC (permalink / raw)
To: Chen Hanxiao; +Cc: mdroth, qemu-devel
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
2017-09-29 21:31 ` Tomáš Golembiovský
@ 2017-10-25 21:58 ` Michael Roth
2017-10-26 9:27 ` Chen Hanxiao
2017-10-26 13:54 ` Tomáš Golembiovský
0 siblings, 2 replies; 10+ messages in thread
From: Michael Roth @ 2017-10-25 21:58 UTC (permalink / raw)
To: Tomáš Golembiovský, Chen Hanxiao; +Cc: qemu-devel
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>
>
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
2017-10-25 21:58 ` Michael Roth
@ 2017-10-26 9:27 ` Chen Hanxiao
2017-10-26 9:59 ` Michael Roth
2017-10-26 13:54 ` Tomáš Golembiovský
1 sibling, 1 reply; 10+ messages in thread
From: Chen Hanxiao @ 2017-10-26 9:27 UTC (permalink / raw)
To: Michael Roth; +Cc: Tomáš Golembiovský, qemu-devel
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>
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
2017-10-26 9:27 ` Chen Hanxiao
@ 2017-10-26 9:59 ` Michael Roth
2017-10-26 11:01 ` Chen Hanxiao
0 siblings, 1 reply; 10+ messages in thread
From: Michael Roth @ 2017-10-26 9:59 UTC (permalink / raw)
To: Chen Hanxiao; +Cc: Tomáš Golembiovský, qemu-devel
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>
> >>
> >
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
2017-10-26 9:59 ` Michael Roth
@ 2017-10-26 11:01 ` Chen Hanxiao
0 siblings, 0 replies; 10+ messages in thread
From: Chen Hanxiao @ 2017-10-26 11:01 UTC (permalink / raw)
To: Michael Roth; +Cc: Tomáš Golembiovský, qemu-devel
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.
>
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
2017-10-25 21:58 ` Michael Roth
2017-10-26 9:27 ` Chen Hanxiao
@ 2017-10-26 13:54 ` Tomáš Golembiovský
2017-10-27 1:05 ` Michael Roth
1 sibling, 1 reply; 10+ messages in thread
From: Tomáš Golembiovský @ 2017-10-26 13:54 UTC (permalink / raw)
To: Michael Roth; +Cc: Chen Hanxiao, qemu-devel
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>
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
2017-10-26 13:54 ` Tomáš Golembiovský
@ 2017-10-27 1:05 ` Michael Roth
2017-11-02 10:58 ` Chen Hanxiao
0 siblings, 1 reply; 10+ messages in thread
From: Michael Roth @ 2017-10-27 1:05 UTC (permalink / raw)
To: Tomáš Golembiovský; +Cc: Chen Hanxiao, qemu-devel
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>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
2017-10-27 1:05 ` Michael Roth
@ 2017-11-02 10:58 ` Chen Hanxiao
2017-11-03 11:16 ` Tomáš Golembiovský
0 siblings, 1 reply; 10+ messages in thread
From: Chen Hanxiao @ 2017-11-02 10:58 UTC (permalink / raw)
To: Michael Roth; +Cc: Tomáš Golembiovský, qemu-devel
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>
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed
2017-11-02 10:58 ` Chen Hanxiao
@ 2017-11-03 11:16 ` Tomáš Golembiovský
0 siblings, 0 replies; 10+ messages in thread
From: Tomáš Golembiovský @ 2017-11-03 11:16 UTC (permalink / raw)
To: Chen Hanxiao; +Cc: Michael Roth, qemu-devel
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-11-03 11:17 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-29 9:11 [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed Chen Hanxiao
2017-09-29 21:31 ` Tomáš Golembiovský
2017-10-25 21:58 ` Michael Roth
2017-10-26 9:27 ` Chen Hanxiao
2017-10-26 9:59 ` Michael Roth
2017-10-26 11:01 ` Chen Hanxiao
2017-10-26 13:54 ` Tomáš Golembiovský
2017-10-27 1:05 ` Michael Roth
2017-11-02 10:58 ` Chen Hanxiao
2017-11-03 11:16 ` Tomáš Golembiovský
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.