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

[Victor Toso]

Hi,

Thanks for taking a look :)

On Thu, Mar 02, 2017 at 10:59:12AM +0100, Christophe Fergeau wrote:
> On Thu, Mar 02, 2017 at 10:23:45AM +0100, Victor Toso wrote:
> > Hi,
> >
> > On Tue, Feb 28, 2017 at 12:21:45PM +0100, Victor Toso wrote:
> > > From: Victor Toso <me at victortoso.com>
> > >
> > > Hi,
> > >
> > > v2->v3:
> > > * Breaking spice_channel_read_wire() into smaller changes. (teuf)
> > >
> > > v2: https://lists.freedesktop.org/archives/spice-devel/2017-February/035455.html
> > > v1: https://lists.freedesktop.org/archives/spice-devel/2017-February/035446.html
> > >
> > > Cheers,
> > >
> > > Victor Toso (6):
> > >   spice-channel: move out non blocking logic of _read_wire()
> > >   spice-channel: move out non blocking logic of _flush_wire()
> > >   spice_channel_read_wire: prefer while(TRUE) instead of goto
> > >   spice_channel_read_wire: move variables to internal scope
> >
> > Any feedback on 3/6 and 4/6 ? I don't mind dropping the 5/6 and 6/6
> > patches at all. They don't bring much benefit and it might be just
> > personal preference.
>
> I was looking at this series yesterday, and I'm not really convinced it
> brings much, the while (TRUE) which is iterated once at most is as weird
> as the goto, before the patch the code is

We should only use goto to deal with errors or exceptions. Not sure why
here would be an exception.

https://www.spice-space.org/spice-project-coding-style-and-coding-conventions.html

> ret = xxxx
> /* error handling depending on 'ret' value */
> if (ret == xxx) {
> } ...

This is 100% true but related to 5/6 and 6/6 which I'm totally fine in
dropping.

>
> /* nominal case */
>
> This series changes this to emphasize the 'read would block, retry', but
> does not really describe why this is better, the initial organization
> looks better to me.
>
> Christophe

This is in part of bigger change that I'm trying to have finished. With
this unfinished change, the channel might not be *allowed* to write/read
to give proper rate/bandwidth to higher priority channels (like
display/input).

In the situation above, we _could_ iterate in the loop more times.

I agree with you that this was not well explained. And I don't mind to
come back to this patches afterwards.

Thanks again for taking a look,
        toso
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170302/6dded963/attachment.sig>