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@xxxxxxxxxx> 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
Attachment:
pgpoCYxOiUHX0.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel