[PATCH weston] compositor-fbdev: make copy of the device string

[Pekka Paalanen]

On Thu, 28 Apr 2016 16:23:03 +0200
Quentin Glidic <sardemff7+wayland at sardemff7.net> wrote:

> On 28/04/2016 16:08, Pekka Paalanen wrote:
> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> >
> > Ensuring that the pointer to the device path stays valid gets harder and
> > harder with migrating to the libweston-style config handling. Therefore,
> > make a copy of the string, private to struct fbdev_output.
> >
> > Now the pointer passed in to fbdev_output_create() could be freed right
> > after the call returns.
> >
> > Cc: Benoit Gschwind <gschwind at gnu-log.net>
> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > ---
> >  src/compositor-fbdev.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
> > index 19c5e3b..e2f978b 100644
> > --- a/src/compositor-fbdev.c
> > +++ b/src/compositor-fbdev.c
> > @@ -84,7 +84,7 @@ struct fbdev_output {
> >  	struct wl_event_source *finish_frame_timer;
> >
> >  	/* Frame buffer details. */
> > -	const char *device; /* ownership shared with fbdev_parameters */
> > +	char *device;
> >  	struct fbdev_screeninfo fb_info;
> >  	void *fb; /* length is fb_info.buffer_length */
> >
> > @@ -470,7 +470,7 @@ fbdev_output_create(struct fbdev_backend *backend,
> >  		return -1;
> >
> >  	output->backend = backend;
> > -	output->device = device;
> > +	output->device = strdup(device);
> >
> >  	/* Create the frame buffer. */
> >  	fb_fd = fbdev_frame_buffer_open(output, device, &output->fb_info);
> > @@ -554,6 +554,7 @@ out_hw_surface:
> >  	weston_output_destroy(&output->base);
> >  	fbdev_frame_buffer_destroy(output);
> >  out_free:
> > +	free(output->device);
> >  	free(output);
> >
> >  	return -1;
> > @@ -580,6 +581,7 @@ fbdev_output_destroy(struct weston_output *base)
> >  	/* Remove the output. */
> >  	weston_output_destroy(&output->base);
> >
> > +	free(output->device);
> >  	free(output);
> >  }
> >
> > @@ -607,7 +609,7 @@ fbdev_output_reenable(struct fbdev_backend *backend,
> >  	struct fbdev_output *output = to_fbdev_output(base);
> >  	struct fbdev_screeninfo new_screen_info;
> >  	int fb_fd;
> > -	const char *device;
> > +	char *device;
> >
> >  	weston_log("Re-enabling fbdev output.\n");
> >
> > @@ -634,9 +636,10 @@ fbdev_output_reenable(struct fbdev_backend *backend,
> >  		/* Remove and re-add the output so that resources depending on
> >  		 * the frame buffer X/Y resolution (such as the shadow buffer)
> >  		 * are re-initialised. */
> > -		device = output->device;
> > -		fbdev_output_destroy(base);
> > +		device = strdup(output->device);
> > +		fbdev_output_destroy(&output->base);
> >  		fbdev_output_create(backend, device);
> > +		free(device);  
> 
> Maybe:
> device = output->device;
> output->device = NULL;
> To avoid an strdup()?

Hi Quentin,

I did think about that, but if someone wanted to e.g. print the device
string in the destroy function, that would be awkward.

> >  		return 0;
> >  	}
> >  
> 
> 
> Another solution is to have fbdev_output_create() take ownership, so no 
> free() in _reenable(), and move the strdup() to backend_create().

Oh yes, that's true. I'm too lazy to bother if no-one really wants it
to be changed, though. :-)

> Whichever solution you pick (this patch, or one of the variants I 
> suggested):
> 
> Reviewed-by: Quentin Glidic <sardemff7+git at sardemff7.net>
> 

Thanks!
I'll still wait Benoit's ack before landing these.


Cheers,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160428/2441e78d/attachment.sig>