[Spice-devel] [PATCH spice-common] marshaller: track if add_fd() was given -1

[Marc-André Lureau]

Hi

----- Original Message -----
> > 
> > In some cases, it might be worth to be able to send a message with a -1
> > fd, has the protocol permits. Change add_fd/get_fd in order to track
> > if the caller wanted to send -1.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau at gmail.com>
> > ---
> >  common/marshaller.c | 23 +++++++++++++++--------
> >  common/marshaller.h |  3 ++-
> >  2 files changed, 17 insertions(+), 9 deletions(-)
> > 
> > diff --git a/common/marshaller.c b/common/marshaller.c
> > index c967371..417f66b 100644
> > --- a/common/marshaller.c
> > +++ b/common/marshaller.c
> > @@ -87,6 +87,7 @@ struct SpiceMarshaller {
> >      MarshallerItem *items;
> >  
> >      MarshallerItem static_items[N_STATIC_ITEMS];
> > +    bool has_fd;
> >      int fd;
> 
> What about using -1 as not valid and < -1 as not filled in?

Just to get rid of a bool? that sounds more confusing to me, but really don't care that much.

> 
> >  };
> >  
> > @@ -621,19 +622,25 @@ void *spice_marshaller_add_int8(SpiceMarshaller *m,
> > int8_t v)
> >  
> >  void spice_marshaller_add_fd(SpiceMarshaller *m, int fd)
> >  {
> > -    spice_assert(m->fd == -1);
> > +    spice_assert(m->has_fd == false);
> >  
> > -    m->fd = dup(fd);
> > -    if (m->fd == -1) {
> > -        perror("dup");
> > +    m->has_fd = true;
> > +    if (fd != -1) {
> > +        m->fd = dup(fd);
> > +        if (m->fd == -1) {
> > +            perror("dup");
> > +        }
> > +    } else {
> > +        m->fd = -1;
> >      }
> >  }
> >  
> > -int spice_marshaller_get_fd(SpiceMarshaller *m)
> > +bool spice_marshaller_get_fd(SpiceMarshaller *m, int *fd)
> >  {
> > -    int fd = m->fd;
> > +    bool had_fd = m->has_fd;
> >  
> > -    m->fd = -1;
> > +    *fd = m->fd;
> > +    m->has_fd = false;
> >  
> > -    return fd;
> > +    return had_fd;
> >  }
> > diff --git a/common/marshaller.h b/common/marshaller.h
> > index b698b69..5b26ae4 100644
> > --- a/common/marshaller.h
> > +++ b/common/marshaller.h
> > @@ -19,6 +19,7 @@
> >  #ifndef _H_MARSHALLER
> >  #define _H_MARSHALLER
> >  
> > +#include <stdbool.h>
> >  #include <spice/macros.h>
> >  #include <spice/types.h>
> >  #include "mem.h"
> > @@ -67,7 +68,7 @@ void *spice_marshaller_add_int8(SpiceMarshaller *m,
> > int8_t
> > v);
> >  void  spice_marshaller_set_uint32(SpiceMarshaller *m, void *ref, uint32_t
> >  v);
> >  
> >  void  spice_marshaller_add_fd(SpiceMarshaller *m, int fd);
> > -int   spice_marshaller_get_fd(SpiceMarshaller *m);
> > +bool  spice_marshaller_get_fd(SpiceMarshaller *m, int *fd);

However, I'd keep this signature, has it allows a simpler caller to check for it

if (spice_marshaller_get_fd(m, &fd)) {
...
}

> >  
> >  SPICE_END_DECLS
> >  
> 
> Otherwise patch looks good.
> I think however is just question of style and optimization paranoia.
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>