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

[Ryo Munakata]

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.

If we did it, we would need to change the several functions which receive const char * as an arg.
Also, output->device is not intended to be modified, that is why it is const qualified. We love const. (right?)

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.

> > > > +
> > > >  	/* 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;
> > > t
> > > Why NULLing it here? Seem like this would make repeated reenable to
> > > fail, no?
> > 
> > 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?

Thank you.
-- 
Ryo Munakata <ryomnktml at gmail.com>