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

[Bryce Harrington]

On Tue, Mar 17, 2015 at 01:31:44AM +0900, Ryo Munakata wrote:
> 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.

Then why not move the check to immediately after the unref?  It does
seem weird to start on another iteration before exiting the loop.

Also, note that strdup has a failure mode and can return NULL.  That
means udev_get_fbdev can return NULL, so where udev_get_fbdev gets
called you'll need to check for that and handle it appropriately
(i.e. fail out of the init routine.)  You may also want to check the
strdup(devnode) call for the memory fault, and at least perror().

Another approach might be to allow output->device == NULL to indicate
that the system default should be used, and then provide that either via
a constant global literal or a pre-processor define.  Then you can avoid
strdup's of literals, and just have the one strdup for the devnode.

Bryce