Re: [spice-gtk v3 0/6] spice-channel: read/flush wire functions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Thu, Mar 02, 2017 at 01:35:32PM +0100, Christophe Fergeau wrote:
> The code is preexisting

Hm, so I should avoid reworking code that does not fit in newer
style or standards?

> it's arguably error handling

Agreed

> and the while (TRUE) is equally odd if you ask me (mostly because you
> said it would loop at most once anyway).

The iteration should happen only once because g_coroutine_socket_wait()
does wait G_IO_IN/G_IO_OUT. Its very unlikely that after
g_coroutine_socket_wait() returns, we can't read or write.

Still, that's not odd. Not odd at all. Unless `goto reread` give us some
performance gain here, I can't see goto label being preferred to
while(1).

In college I would lose valuable points choosing one over the other.

> I guess I could live with it with a slightly better changelog, and
> maybe a comment indicating this is not really a loop.

I can't see a reason to improve the commit log if you don't agree with
the changes and I don't really intend to keep discussing over this that
seems more a personal preference thing. I might be completely wrong in
the end :)

Still, IMO I see an improvement while interacting with this code later
on. Maybe I should not have sent this to soon but I'm trying to avoid
huge patch series whenever I can. See this 'wip-qos' patch in current
master [1] and with this series [2].

[1] https://gitlab.com/victortoso/spice-gtk/commit/9c68d7875beafbc49df4dc1b8c58a490fbc355b8?view=parallel
[2] https://gitlab.com/victortoso/spice-gtk/commit/326528c9674dbeb765bb50f6d0b729f8a2b45c57?view=parallel

Cheers,
        toso

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]