Re: [Spice-devel] [PATCH spice-server 02/12] Be consistent with opaque type

[Frediano Ziglio]

> 
> On Wed, Oct 26, 2016 at 02:43:22AM -0400, Frediano Ziglio wrote:
> > I think that my reasoning is more focuses on the "get refcounting for free"
> > not being
> > a great design so willing to change and I prefer a compile error in the
> > future
> > instead on the "base type".
> 
> Well, this makes the intent of the code harder to get imo, this falls
> apart as soon as an intermediate class is added, ... so I really don't
> think explicit mentions of the 'base' field in the code is something we
> should encourage. If wrapped in a macro, why not, but this still leaves
> the problem that this only work with one level of inheritance.
> 
> Christophe
> 

I prefer having to change the source instead of spending hours
digging into core files. If base is removed no warnings are issues
and memory corruptions happens.

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

Re: [Spice-devel] [PATCH spice-server 02/12] Be consistent with opaque type

[Christophe Fergeau]

On Wed, Oct 26, 2016 at 02:43:22AM -0400, Frediano Ziglio wrote:
> I think that my reasoning is more focuses on the "get refcounting for free" 
> not being
> a great design so willing to change and I prefer a compile error in the future
> instead on the "base type".

Well, this makes the intent of the code harder to get imo, this falls
apart as soon as an intermediate class is added, ... so I really don't
think explicit mentions of the 'base' field in the code is something we
should encourage. If wrapped in a macro, why not, but this still leaves
the problem that this only work with one level of inheritance.

Christophe

> 
> Frediano
> 
> > 
> > 
> > > 
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  server/reds.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/server/reds.c b/server/reds.c
> > > index 79f9c9e..a71029f 100644
> > > --- a/server/reds.c
> > > +++ b/server/reds.c
> > > @@ -745,7 +745,8 @@ static void reds_agent_remove(RedsState *reds)
> > >  
> > >  static void vdi_port_read_buf_release(uint8_t *data, void *opaque)
> > >  {
> > > -red_pipe_item_unref((RedPipeItem *)opaque);
> > > +RedVDIReadBuf *read_buf = (RedVDIReadBuf *)opaque;
> > > +red_pipe_item_unref(_buf->base);
> > >  }
> > >  
> > >  /*
> > > --
> > > 2.7.4
> > > 
> > > ___
> > > Spice-devel mailing list
> > > Spice-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server 02/12] Be consistent with opaque type

[Frediano Ziglio]

> 
> On Tue, Oct 18, 2016 at 10:09:47AM +0100, Frediano Ziglio wrote:
> > vdi_port_read_buf_release is registered passing data as
> > RedVDIReadBuf*, not RedPipeItem*. Cast opaque to proper
> > pointer type to avoid the assumption that first field of
> > RedVDIReadBuf is a RedPipeItem.
> 
> My initial objection still stands here, RedVDIReadBuf has a 'base'
> field as its first member, and it's very much meant to be a RedPipeItem
> as a way to get refcounting for free. So I much prefer the current
> version where we have a cast from child to base type, rather than
> apparently unref'ing something contained in the read_buf object.
> 
> Christophe
> 

I replied quite a month ago, see 
https://lists.freedesktop.org/archives/spice-devel/2016-September/032360.html.
I took the not reply as a neutral position from you and I posted again the 
patch twice and
at the end was acked so I accepted the ack and merged it.
I think that my reasoning is more focuses on the "get refcounting for free" not 
being
a great design so willing to change and I prefer a compile error in the future
instead on the "base type".

Frediano

> 
> 
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/reds.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index 79f9c9e..a71029f 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -745,7 +745,8 @@ static void reds_agent_remove(RedsState *reds)
> >  
> >  static void vdi_port_read_buf_release(uint8_t *data, void *opaque)
> >  {
> > -red_pipe_item_unref((RedPipeItem *)opaque);
> > +RedVDIReadBuf *read_buf = (RedVDIReadBuf *)opaque;
> > +red_pipe_item_unref(_buf->base);
> >  }
> >  
> >  /*
> > --
> > 2.7.4
> > 
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server 02/12] Be consistent with opaque type

[Christophe Fergeau]

On Tue, Oct 18, 2016 at 10:09:47AM +0100, Frediano Ziglio wrote:
> vdi_port_read_buf_release is registered passing data as
> RedVDIReadBuf*, not RedPipeItem*. Cast opaque to proper
> pointer type to avoid the assumption that first field of
> RedVDIReadBuf is a RedPipeItem.

My initial objection still stands here, RedVDIReadBuf has a 'base'
field as its first member, and it's very much meant to be a RedPipeItem
as a way to get refcounting for free. So I much prefer the current
version where we have a cast from child to base type, rather than
apparently unref'ing something contained in the read_buf object.

Christophe



> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/reds.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/server/reds.c b/server/reds.c
> index 79f9c9e..a71029f 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -745,7 +745,8 @@ static void reds_agent_remove(RedsState *reds)
>  
>  static void vdi_port_read_buf_release(uint8_t *data, void *opaque)
>  {
> -red_pipe_item_unref((RedPipeItem *)opaque);
> +RedVDIReadBuf *read_buf = (RedVDIReadBuf *)opaque;
> +red_pipe_item_unref(_buf->base);
>  }
>  
>  /*
> -- 
> 2.7.4
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server 02/12] Be consistent with opaque type

[Pavel Grunt]

On Tue, 2016-10-18 at 10:09 +0100, Frediano Ziglio wrote:
> vdi_port_read_buf_release is registered passing data as
> RedVDIReadBuf*, not RedPipeItem*. Cast opaque to proper
> pointer type to avoid the assumption that first field of
> RedVDIReadBuf is a RedPipeItem.
> 
> Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
> ---
>  server/reds.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/server/reds.c b/server/reds.c
> index 79f9c9e..a71029f 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -745,7 +745,8 @@ static void reds_agent_remove(RedsState *reds)
>  
>  static void vdi_port_read_buf_release(uint8_t *data, void *opaque)
>  {
> -red_pipe_item_unref((RedPipeItem *)opaque);
> +RedVDIReadBuf *read_buf = (RedVDIReadBuf *)opaque;
> +red_pipe_item_unref(_buf->base);
>  }
>  
>  /*
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel