[PATCH v5 5/7] libweston: fbdev: Attempt to detect the first framebuffer device in the seat. instead of defaulting to /dev/fb0

[Pekka Paalanen]

On Tue, 23 Jan 2018 00:06:36 -0500
nerdopolis <bluescreen_avenger at verizon.net> wrote:

> On Monday, January 22, 2018 4:50:35 AM EST Pekka Paalanen wrote:
> > On Fri, 29 Dec 2017 13:31:51 -0500
> > nerdopolis <bluescreen_avenger at verizon.net> wrote:
> >   
> > > This adds a function to detect the first framebuffer device in the
> > > current seat. Instead of hardcoding /dev/fb0, use udev to find the
> > > first framebuffer device in the seat.
> > > ---
> > >  libweston/compositor-fbdev.c | 45 +++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 42 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/libweston/compositor-fbdev.c b/libweston/compositor-fbdev.c
> > > index 39668aa8..34e6c2f3 100644
> > > --- a/libweston/compositor-fbdev.c
> > > +++ b/libweston/compositor-fbdev.c
> > > @@ -711,6 +711,42 @@ fbdev_restore(struct weston_compositor *compositor)
> > >  	weston_launcher_restore(compositor->launcher);
> > >  }
> > >  
> > > +static char *
> > > +find_framebuffer_device(struct fbdev_backend *b, const char *seat)
> > > +{
> > > +	struct udev_enumerate *e;
> > > +	struct udev_list_entry *entry;
> > > +	const char *path, *device_seat;
> > > +	char *fb_device;
> > > +	struct udev_device *device;
> > > +
> > > +	e = udev_enumerate_new(b->udev);
> > > +	udev_enumerate_add_match_subsystem(e, "graphics");
> > > +	udev_enumerate_add_match_sysname(e, "fb[0-9]*");
> > > +
> > > +	udev_enumerate_scan_devices(e);
> > > +	fb_device = NULL;
> > > +	udev_list_entry_foreach(entry, udev_enumerate_get_list_entry(e)) {
> > > +
> > > +		path = udev_list_entry_get_name(entry);
> > > +		device = udev_device_new_from_syspath(b->udev, path);
> > > +		if (!device)
> > > +			continue;
> > > +		device_seat = udev_device_get_property_value(device, "ID_SEAT");
> > > +		if (!device_seat)
> > > +			device_seat = default_seat;
> > > +		if (!strcmp(device_seat, seat)) {
> > > +			fb_device = udev_device_get_devnode(device);
> > > +			udev_enumerate_unref(e);  
> > 
> > Hi,
> > 
> > seems like this should be udev_device_unref() instead, otherwise
> > 'e' is unreffed twice and 'device' is leaked.
> >   
> Patch 7 improves this function somewhat, but I don't really know how to squash 5 and 7 into
> 5 without squashing 6... ...I guess I could just copy and paste that manually...


Hi,

what a later patch does is quite irrelevant if this patch introduces a
problem. Every commit in the history must not regress anything as far
as we can tell. This is to ensure bisectability of the history: when
bisecting for a bug, one would rather not hit a different bug along the
way.

I idea is that you would fix patch 5 as is, and then rebase the later
patches on top with approriate modifications. No squashing intended.

> 
> With patch 7 applied on top of this, is the only issue that I just need to do
>                        fb_device = strdup(find_device);
>                        udev_device_unref(device);
> to be able to unref device ? 

I think so, along with the needed free()s to not leak it, if the udev
API works as it would appear.

> > > +			break;
> > > +		}
> > > +		udev_device_unref(device);
> > > +	}
> > > +
> > > +	udev_enumerate_unref(e);
> > > +	return fb_device;
> > > +}
> > > +
> > >  static struct fbdev_backend *
> > >  fbdev_backend_create(struct weston_compositor *compositor,
> > >                       struct weston_fbdev_backend_config *param)
> > > @@ -743,6 +779,11 @@ fbdev_backend_create(struct weston_compositor *compositor,
> > >  		goto out_compositor;
> > >  	}
> > >  
> > > +	if (!param->device)
> > > +		param->device=find_framebuffer_device(backend, seat_id);
> > > +	if (!param->device)
> > > +		param->device=strdup("/dev/fb0");  
> > 
> > Missing spaces around '=' on both lines.
> > 
> > The assigned string is leaked, there is no free() for it. However, one
> > must take care to not free() the string that came as an argument via
> > weston_fbdev_backend_config struct.
> >   
> Not sure what I need to do here, regarding the free() TBH.

You need to ensure that dynamically allocated strings get free()'d.

> The issue is with the param->device=strdup("/dev/fb0"); line correct?
> Is the issue that the hardcoded "/dev/fb0" string that is strdup'ed stuck in memory?

No. If you strdup() anything, you need to make sure to free() the
result when it is no longer needed. strdup() effectively calls malloc().

However, there are also code paths where a pointer to a statically
allocated string (string literal) is assigned to the variable. Calling
free() on such a pointer is an error.

Since tracking whether the pointed string was allocated statically or
dynamically is inconvenient, it is often a good idea to ensure the
string is always dynamically allocated, so that free() can be called
unconditionally. This means that in the static string path one would
just add a strdup().

(The discussion about static vs. dynamic allocation is little of a
simplification here. Statically allocated really means "not owned
by this piece of code". E.g. function arguments may point to
dynamically allocated data, but it does not necessarily mean the
function is supposed to free() them.)


Thanks,
pq

> > > +
> > >  	/* Set up the TTY. */
> > >  	backend->session_listener.notify = session_notify;
> > >  	wl_signal_add(&compositor->session_signal,
> > > @@ -790,10 +831,8 @@ out_compositor:
> > >  static void
> > >  config_init_to_defaults(struct weston_fbdev_backend_config *config)
> > >  {
> > > -	/* TODO: Ideally, available frame buffers should be enumerated using
> > > -	 * udev, rather than passing a device node in as a parameter. */
> > >  	config->tty = 0; /* default to current tty */
> > > -	config->device = "/dev/fb0"; /* default frame buffer */
> > > +	config->device = NULL;
> > >  	config->seat_id = NULL;
> > >  }
> > >    
> > 
> > Otherwise looks good to me.
> > 
> > 
> > Thanks,
> > pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180123/998e25a3/attachment.sig>