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

[Tomáš Golembiovský]

On Thu, 2 Nov 2017 18:58:49 +0800 (CST)
"Chen Hanxiao" <address@hidden> wrote:

> At 2017-10-27 09:05:25, "Michael Roth" <address@hidden> wrote:
> >Quoting Tomáš Golembiovský (2017-10-26 08:54:37)  
> >> On Wed, 25 Oct 2017 16:58:07 -0500
> >> Michael Roth <address@hidden> wrote:
> >>   
> >> > Quoting Tomáš Golembiovský (2017-09-29 16:31:02)  
> >> > > On Fri, 29 Sep 2017 17:11:22 +0800
> >> > > Chen Hanxiao <address@hidden> wrote:
> >> > > 
> >> > > Reviewed-by: Tomáš Golembiovský <address@hidden>    
> >> > 
> >> > 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ý <address@hidden>
> >>   
> >  


-- 
Tomáš Golembiovský <address@hidden>