[Spice-devel] [spice-xpi 4/5] Add SpiceControllerWin class

[Christophe Fergeau]

Hey, thanks for the detailed review!

A few questions below,

On Sun, Mar 24, 2013 at 11:27:45PM +0100, Marc-André Lureau wrote:
> On Sun, Mar 24, 2013 at 12:16 PM, Christophe Fergeau
> <cfergeau at redhat.com> wrote:
> > +SpiceControllerWin::~SpiceControllerWin()
> > +{
> > +}
> > +
> > +int SpiceControllerWin::Connect()
> > +{
> > +    HANDLE hClientPipe;
> > +
> > +    hClientPipe = CreateFile(m_name.c_str(),
> > +                             GENERIC_READ |  GENERIC_WRITE,
> > +                             0, NULL,
> > +                             OPEN_EXISTING,
> > +                             SECURITY_SQOS_PRESENT | SECURITY_ANONYMOUS,
> > +                             NULL);
> 
> I would simplify a little, just g_return_val_if_fail(hClientPipe !=
> INVALID_HANDLE_VALUE, -1) here instead of extra conditions and
> branches below.
> 
> > +    if (hClientPipe != INVALID_HANDLE_VALUE) {
> > +        g_warning("Connection OK");
> > +        m_pipe = g_win32_output_stream_new(hClientPipe, TRUE);
> > +        return 0;
> > +    } else {
> > +        g_warning("Connection failed");
> > +        return -1;
> > +    }
> > +    g_return_val_if_reached(-1);
> > +}
> 
>  In general, returning true on success is easier to read.

This mimics the return value of connect(2), I'd prefer to keep that (at
least in that patch, can be improved as a separate cleanup as this needs
changes in SpiceController and SpiceControllerUnix as well).

> > +//checks whether the handle owner is the current user.
> > +static bool is_same_user(HANDLE handle)
> > +{
> > +    PSECURITY_DESCRIPTOR psec_desc_handle = NULL;
> > +    PSECURITY_DESCRIPTOR psec_desc_user = NULL;
> > +    PSID psid_handle;
> > +    PSID psid_user;
> > +    bool ret;
> > +
> > +    ret = !get_sid(handle, &psid_handle, &psec_desc_handle) &&
> > +          !get_sid(GetCurrentProcess(), &psid_user, &psec_desc_user) &&
> > +          EqualSid(psid_handle, psid_user);
> 
> That would make this easier to read.

'That' ? Sorry didn't get what you mean here.

> 
> > +    LocalFree(psec_desc_handle);
> > +    LocalFree(psec_desc_user);
> > +    return ret;
> > +}
> > +
> > +bool SpiceControllerWin::CheckPipe()
> > +{
> > +    void *hClientPipe;
> > +
> > +    if (!G_IS_WIN32_OUTPUT_STREAM(m_pipe))
> > +        return false;
> 
> g_return_val_if_fail(G_IS_WIN32_OUTPUT_STREAM(m_pipe), FALSE) is more idiomatic.
> 
> > +    hClientPipe = g_win32_output_stream_get_handle(G_WIN32_OUTPUT_STREAM(m_pipe));
> > +    // Verify the named-pipe-server owner is the current user.
> > +    // Do it here so upon failure use next condition cleanup code.
> > +    if (hClientPipe != INVALID_HANDLE_VALUE) {
> 
> Can this happen? If not, it would be nicer to have g_return
> pre-condition for that.

g_return pre-condition? Do you mean this:

hClientPipe = g_win32_output_stream_get_handle(G_WIN32_OUTPUT_STREAM(m_pipe));
g_return_if_fail(hClientPipe != INVALID_HANDLE_VALUE);

?

Thanks,

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20130325/9af68e9e/attachment.pgp>