[Spice-devel] [PATCH 09/22] Move read, write, handle and locking into the 'Stream' class

[Christophe Fergeau]

On Thu, Mar 01, 2018 at 08:52:50PM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 1 Mar 2018, at 11:59, Frediano Ziglio <fziglio at redhat.com> wrote:
> > 
> >> 
> >> From: Christophe de Dinechin <dinechin at redhat.com>
> >> 
> >> The 'Stream' class is designed to abstract file I/O. In a subsequent
> >> patch, message formatting will be isolated out of the class, but in
> >> order to minimize code changes, this intermediate step simply moves
> >> the corresponding functions within the Stream class.
> >> 
> >> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> >> ---
> >> src/spice-streaming-agent.cpp | 108
> >> +++++++++++++++++++++++-------------------
> >> 1 file changed, 58 insertions(+), 50 deletions(-)
> >> 
> >> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> >> index 21f9c31..4d24234 100644
> >> --- a/src/spice-streaming-agent.cpp
> >> +++ b/src/spice-streaming-agent.cpp
> >> @@ -40,8 +40,6 @@
> >> 
> >> using namespace spice::streaming_agent;
> >> 
> >> -static size_t write_all(int fd, const void *buf, const size_t len);
> >> -
> >> static ConcreteAgent agent;
> >> 
> >> namespace spice
> >> @@ -72,31 +70,44 @@ class Stream
> >> public:
> >>     Stream(const char *name)
> >>     {
> >> -        fd = open(name, O_RDWR);
> >> -        if (fd < 0) {
> >> +        streamfd = open(name, O_RDWR);
> >> +        if (streamfd < 0) {
> > 
> > is inside Stream, no reason to have a "streamfd" there, "fd" was fine.
> 
> while we have ‘streamfd’ and ‘pollfd’ in the same file, I find it clearer that way.

One is a class member, the other is a local variable name used in one
mothed, if the naming is not clear, I don't think it's 'fd' which needs
to be renamed.

> >> private:
> >> -    int fd = -1;
> >> +    int streamfd = -1;
> >> +    std::mutex mutex;
> >> };
> >> 
> >> }} // namespace spice::streaming_agent
> >> 
> >> -
> > 
> > spurious line removal, better to stick to 1 line spacing.
> 
> can’t have both, this spurious line removal is to stick with one line spacing.

This was introduced in "Since we use a namespace, simplify name of local
classes" (and pointed out by Frediano), so yes you can have both ;)

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180308/82f5fbe1/attachment-0001.sig>