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

[Bryce Harrington]

On Tue, Mar 17, 2015 at 01:26:32AM +0900, Ryo Munakata wrote:
> On Wed, 11 Mar 2015 17:54:42 +0000
> Philip Withnall <philip at tecnocode.co.uk> wrote:
> 
> First, sorry for my late reply.
> 
> > On Thu, 2015-03-12 at 00:40 +0900, Ryo Munakata wrote:
> > > On Wed, 11 Mar 2015 16:05:55 +0200
> > > Pekka Paalanen <ppaalanen at gmail.com> 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.
> > > > 
> > > > > 
> > > > > 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.
> > > 
> > > This cast causes a warning like:
> > >  src/compositor-fbdev.c:696:7: warning: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type
> > > I think we need it for silencing the warning.
> > > What do you think?
> > > 
> > > And yes, that if statement is unnecessary. I'll fix it.
> > 
> > Rather than casting, you should remove the const qualifier from struct
> > fbdev_output.device.

+1 agreed with Philip.

> If we did it, we would need to change the several functions which receive const char * as an arg.

Nope, functions which have a const char* arg can take a non-const arg.
They're just not able to change it.

> Also, output->device is not intended to be modified, that is why it is const qualified. We love const. (right?)

Your patch implements the machinery to modify it, so it's time to change
the structure accordingly.

> And furthermore, it is easier to just do free((char *)output->device) than removing the const qualifier.
> The return type of strdup() is char *, so, in this case, casting between char* and const char * is not a problem, I think.

I don't think easy should factor into this.  :-)

Generally, using casts to remove consts is a signal of a break in the
design.  I spotted the same issue independently before reading Philip's
post.

> > > > > +
> > > > >  	/* 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?

There's a leak here as well.  We copy the pointer to device, which is
then passed as the second param to fbdev_output_create() but then is
never free'd.

This patch sets output->device to NULL to sidestep the free() that was
added in fbdev_output_destroy(), so it has to take care of the free()
itself when done with device.

> > > 
> > > Here we reuse output->device for the new output. (device = output->device;)
> > > output->device is allocated by strdup() dynamically and fbdev_output_destroy() frees it if not NULL.
> > > So if you don't set output->device as NULL we need to reallocate and copy the entire device string for the new output everytime this function is called.
> > > This is why I set output->device as NULL to prevent it.
> > That seems like a hacky way of doing things. You would be better off
> > refactoring the code out of fbdev_output_create() which initalises the
> > struct fbdev_output (after it’s allocated), and just calling that here.
> 
> Hmm, I don't get it.
> Just setting NULL to prevent the further free()s is so hacky way? I think it's very common way.
> And how would refactor the code to do that easier than the way what I wrote?

Again, "easy" is irrelevant; "robust" is the proper goal.  :-)

Currently fbdev_output_create() both allocates and initializes all the
elements of a new fbdev_output.  What he's suggesting is to split out a
fbdev_output_init() routine which zeros out the structure and sets
compositor and device, sets the output details and transform, and so
on.  (Fwiw, this doesn't look like it would be 'easy' to do, but being
able to skip memory allocation would make output reenablement work
more efficiently.)

I think in this scheme, we'd no longer have a .device entry at all in
the fbdev_parameters.  Instead of calling the new udev_get_fbdev in
fbdev_compositor_create(), you move that call into either your new
fbdev_output_init() routine, or to fbdev_output_create().  (The former
if you want udev re-queried when re-enabling the output; the latter if
you want udev queried only once and then the value reused.)

Bryce