[Spice-devel] [PATCH spice-server v3 4/5] red-channel-capabilities: Enhance

[Frediano Ziglio]

> 
> On Thu, Mar 02, 2017 at 09:16:10AM +0000, Frediano Ziglio wrote:
> > Add function to initialize and destroy this type.
> > Add GObject type for boxing it.
> 
> Nit: I would not call 'GObject type' a GType/GBoxed GObject.
> 
> 
> > These changes a in preparation for next patch.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/red-channel-capabilities.c | 55
> >  +++++++++++++++++++++++++++++++++++++++
> >  server/red-channel-capabilities.h | 16 +++++++++++-
> >  2 files changed, 70 insertions(+), 1 deletion(-)
> > 
> > diff --git a/server/red-channel-capabilities.c
> > b/server/red-channel-capabilities.c
> > index 39bde66..dc28298 100644
> > --- a/server/red-channel-capabilities.c
> > +++ b/server/red-channel-capabilities.c
> > @@ -18,5 +18,60 @@
> >  #include <config.h>
> >  #endif
> >  
> > +#include <string.h>
> > +#include <common/mem.h>
> > +#include <common/macros.h>
> > +
> >  #include "red-channel-capabilities.h"
> >  
> > +GType red_channel_capabilities_type;
> > +
> > +void red_channel_capabilities_init(RedChannelCapabilities *dest,
> > +                                   const RedChannelCapabilities *caps)
> > +{
> > +    *dest = *caps;
> > +    if (caps->common_caps) {
> > +        dest->common_caps = spice_memdup(caps->common_caps,
> > +                                         caps->num_common_caps *
> > sizeof(uint32_t));
> > +    }
> > +    if (caps->num_caps) {
> > +        dest->caps = spice_memdup(caps->caps, caps->num_caps *
> > sizeof(uint32_t));
> > +    }
> > +}
> > +
> > +void red_channel_capabilities_reset(RedChannelCapabilities *caps)
> > +{
> > +    if (caps->num_common_caps) {
> > +        free(caps->common_caps);
> > +    }
> > +
> > +    if (caps->num_caps) {
> > +        free(caps->caps);
> > +    }

Does it actually make sense the if?
Maybe free is enough, if the pointer is valid is supposed to
came from a malloc.

> > +    memset(caps, 0, sizeof(*caps));
> > +}
> > +
> > +static RedChannelCapabilities *red_channel_capabilities_dup(const
> > RedChannelCapabilities *caps)
> > +{
> > +    if (!caps) {
> > +        return NULL;
> > +    }
> 
> It's apparently invalid to return NULL from a GBoxedCopyFunc. Dunno if
> the 'caps' argument can validly be NULL or not, probably not?
> 
> > +
> > +    RedChannelCapabilities *res = spice_new(RedChannelCapabilities, 1);
> > +    red_channel_capabilities_init(res, caps);
> > +    return res;
> > +}
> > +
> > +static void red_channel_capabilities_free(RedChannelCapabilities *caps)
> > +{
> > +    red_channel_capabilities_reset(caps);
> > +    free(caps);
> > +}
> > +
> > +SPICE_CONSTRUCTOR_FUNC(red_channel_capabilities_construct)
> > +{
> > +    red_channel_capabilities_type =
> > +        g_boxed_type_register_static("red_channel_capabilities_type",
> > +                                     (GBoxedCopyFunc)
> > red_channel_capabilities_dup,
> > +                                     (GBoxedFreeFunc)
> > red_channel_capabilities_free);
> > +}
> > diff --git a/server/red-channel-capabilities.h
> > b/server/red-channel-capabilities.h
> > index 8729134..1f52cd4 100644
> > --- a/server/red-channel-capabilities.h
> > +++ b/server/red-channel-capabilities.h
> > @@ -20,10 +20,13 @@
> >  #define RED_CHANNEL_CAPABILITIES_H_
> >  
> >  #include <stdint.h>
> > -#include <glib.h>
> > +#include <glib-object.h>
> >  
> >  G_BEGIN_DECLS
> >  
> > +/* Holds channel capabilities.
> > + * Channel capabilities are composed by a common part
> > + * and a specific one. */
> >  typedef struct RedChannelCapabilities {
> >      int num_common_caps;
> >      uint32_t *common_caps;
> > @@ -31,6 +34,17 @@ typedef struct RedChannelCapabilities {
> >      uint32_t *caps;
> >  } RedChannelCapabilities;
> >  
> > +/* Initialize the structure based on a previous one. */
> > +void red_channel_capabilities_init(RedChannelCapabilities *dest,
> > +                                   const RedChannelCapabilities *caps);
> > +
> > +/* Reset capabilities.
> > + * All resources are freed by this function. */
> > +void red_channel_capabilities_reset(RedChannelCapabilities *caps);
> > +
> > +/* GObject type that can be used to box RedChannelCapabilities */
> > +extern GType red_channel_capabilities_type;
> 
> Usually a _get_type() method is exported, together with a #define.
> You favoured using a static constructor, but I'd still add a
> #define RED_TYPE_CHANNEL_CAPABILITIES red_channel_capabilities_type
> for consistency with the other GType names.
> 
> 
> Looks good to me.
> 
> Christophe
> 
Frediano