[PATCH weston] compositor-fbdev: enumerate available fbdevs using udev

[Ryo Munakata]

On Wed, 11 Mar 2015 17:58:00 +0000
Philip Withnall <philip at tecnocode.co.uk> wrote:

> On Wed, 2015-03-11 at 16:05 +0200, Pekka Paalanen wrote:
> > On Tue, 10 Mar 2015 11:34:45 +0900
> > Ryo Munakata <ryomnktml at gmail.com> wrote:
> > 
> > > This was a TODO:
> > > "Ideally, available frame buffers should be enumerated
> > > using udev, rather than passing a device node in as a
> > > parameter."
> > 
> > Hi,
> > 
> > I'm CC'ing the person responsible for the TODO-comment.
> 
> .o/
> 
> > > Signed-off-by: Ryo Munakata <ryomnktml at gmail.com>
> > > ---
> > >  src/compositor-fbdev.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 52 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
> > > index eedf252..c0e6985 100644
> > > --- a/src/compositor-fbdev.c
> > > +++ b/src/compositor-fbdev.c
> > > @@ -46,6 +46,8 @@
> > >  #include "gl-renderer.h"
> > >  #include "presentation_timing-server-protocol.h"
> > >  
> > > +#define FBDEV_MAJOR_NUMBER (29)
> > > +
> > >  struct fbdev_compositor {
> > >  	struct weston_compositor base;
> > >  	uint32_t prev_state;
> > > @@ -690,6 +692,9 @@ fbdev_output_destroy(struct weston_output *base)
> > >  		gl_renderer->output_destroy(base);
> > >  	}
> > >  
> > > +	if (output->device)
> > > +		free((char *)output->device);
> > 
> > No need to cast. Also no need for 'if' because free(NULL) is perfectly
> > legal.
> > 
> > > +
> > >  	/* Remove the output. */
> > >  	weston_output_destroy(&output->base);
> > >  
> > > @@ -748,6 +753,7 @@ fbdev_output_reenable(struct fbdev_compositor *compositor,
> > >  		 * the frame buffer X/Y resolution (such as the shadow buffer)
> > >  		 * are re-initialised. */
> > >  		device = output->device;
> > > +		output->device = NULL;
> > 
> > Why NULLing it here? Seem like this would make repeated reenable to
> > fail, no?
> > 
> > >  		fbdev_output_destroy(base);
> > >  		fbdev_output_create(compositor, device);
> > >  
> > > @@ -795,6 +801,7 @@ fbdev_compositor_destroy(struct weston_compositor *base)
> > >  	struct fbdev_compositor *compositor = to_fbdev_compositor(base);
> > >  
> > >  	udev_input_destroy(&compositor->input);
> > > +	udev_unref(compositor->udev);
> > >  
> > >  	/* Destroy the output. */
> > >  	weston_compositor_shutdown(&compositor->base);
> > > @@ -861,6 +868,47 @@ switch_vt_binding(struct weston_seat *seat, uint32_t time, uint32_t key, void *d
> > >  	weston_launcher_activate_vt(compositor->launcher, key - KEY_F1 + 1);
> > >  }
> > >  
> > > +static char *
> > > +udev_get_fbdev(struct udev *udev)
> > > +{
> > > +	char *fbdev = NULL;
> > > +	struct udev_enumerate *enumerate;
> > > +	struct udev_list_entry *list, *entry;
> > > +
> > > +	if (!(enumerate = udev_enumerate_new(udev)))
> > > +		goto err;
> > > +
> > > +	udev_enumerate_add_match_subsystem(enumerate, "graphics");
> > > +	udev_enumerate_scan_devices(enumerate);
> > > +
> > > +	list = udev_enumerate_get_list_entry(enumerate);
> > > +	udev_list_entry_foreach(entry, list) {
> > > +		const char *devnode;
> > > +		const char *syspath = udev_list_entry_get_name(entry);
> > > +		struct udev_device *device;
> > > +
> > > +		if (fbdev)
> > > +			break;
> 
> Why break here? Why not break where you actually assign to fbdev, just
> below?
> 
> > > +
> > > +		if (!(device = udev_device_new_from_syspath(udev, syspath)))
> > > +			continue;
> > > +
> > > +		devnode = udev_device_get_devnode(device);
> > > +		if (devnode) {
> > > +			dev_t devnum = udev_device_get_devnum(device);
> > > +
> > > +			if (major(devnum) == FBDEV_MAJOR_NUMBER)
> > > +				fbdev = strdup(devnode);
> > > +		}
> > > +
> > > +		udev_device_unref(device);
> > > +	}
> > > +
> > > +	udev_enumerate_unref(enumerate);
> > > +err:
> > > +	return fbdev ? fbdev : strdup("/dev/fb0");
> > > +}

If we break where we actually assign to fbdev, *device will leak because udev_device_unref(device) will never be called.
This is a reason.

Thanks.
-- 
Ryo Munakata <ryomnktml at gmail.com>