[PATCH V3 5/7] weston:Add the weston randr protocol implementation

[Wang, Quanxian]

Sorry to be later. Thanks for your comment.
On Wed, 2014-04-09 at 09:59 +0200, Hardening wrote:
> Le 08/04/2014 07:03, Quanxian Wang a écrit :
> > Signed-off-by: Quanxian Wang <quanxian.wang at intel.com>
> > ---
> >   module/Makefile.am        |    3 +
> >   module/wrandr/Makefile.am |   32 ++
> >   module/wrandr/wrandr.c    | 1262 +++++++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 1297 insertions(+)
> >   create mode 100644 module/Makefile.am
> >   create mode 100644 module/wrandr/Makefile.am
> >   create mode 100644 module/wrandr/wrandr.c
> >
> > diff --git a/module/Makefile.am b/module/Makefile.am new file mode 
> > 100644 index 0000000..1a10e65
> > --- /dev/null
> > +++ b/module/Makefile.am
> > @@ -0,0 +1,3 @@
> > +SUBDIRS =
> > +
> > +SUBDIRS += wrandr
> > diff --git a/module/wrandr/Makefile.am b/module/wrandr/Makefile.am 
> > new file mode 100644 index 0000000..b0f2e6b
> > --- /dev/null
> > +++ b/module/wrandr/Makefile.am
> > @@ -0,0 +1,32 @@
> > +moduledir = $(libdir)/weston
> > +module_LTLIBRARIES = $(wrandr)
> > +
> > +AM_CPPFLAGS =					\
> > +	-I$(top_srcdir)/shared			\
> > +	-I$(top_srcdir)/src			\
> > +	-I$(top_builddir)/src			\
> > +	-DDATADIR='"$(datadir)"'		\
> > +	-DMODULEDIR='"$(moduledir)"'		\
> > +	-DLIBEXECDIR='"$(libexecdir)"'		\
> > +	-DIN_WESTON
> > +
> 
> [...]
> 
> > +static void
> > +update_region(struct weston_compositor *ec,
> > +	      struct weston_output *target_output) {
> > +	pixman_region32_t old_output_region;
> > +
> > +	/* Update output region and transformation matrix. */
> > +	pixman_region32_init(&old_output_region);
> > +	pixman_region32_copy(&old_output_region, &target_output->region);
> > +	weston_output_transform_scale_init(target_output,
> > +					   target_output->transform,
> > +					   target_output->current_scale);
> > +
> > +	pixman_region32_init(&target_output->previous_damage);
> > +	pixman_region32_init_rect(&target_output->region,
> > +				  target_output->x, target_output->y,
> > +				  target_output->width, target_output->height);
> > +
> > +	weston_output_update_matrix(target_output);
> > +
> > +	adjust_pointer(target_output, &old_output_region);
> > +	pixman_region32_fini(&old_output_region);
> > +
> > +	/* Damage the output region in primary_plane */
> > +	pixman_region32_union(&ec->primary_plane.damage,
> > +			      &ec->primary_plane.damage,
> > +			      &target_output->region);
> > +
> > +	target_output->dirty = 1;
> > +}
> > +
> > +static void
> > +randr_switch_mode(struct randr_output_request *request,
> > +		  uint32_t *results)
> > +{
> > +	time_t time = 0;
> > +	int i = 0, found = 0;
> > +	int result = WRANDR_TYPE_RET_NOACT;
> > +	int timing_mode = 0;
> > +	int move_bits = 0;
> > +	struct weston_mode *mode;
> > +	struct weston_output *output = request->output;
> > +
> > +	if (request->flags & 1<<WRANDR_TYPE_OOP_MODENUM) {
> > +		timing_mode = 1;
> > +		time = request->modenum.modi_time;
> > +		*results |= result<<
> > +			   (WRANDR_TYPE_OOP_MODENUM * 2);
> > +	}
> > +
> > +	if (request->flags & 1<<WRANDR_TYPE_OOP_MODE) {
> > +		*results |= result<<
> > +			   (WRANDR_TYPE_OOP_MODE * 2);
> > +		if (request->mode->modi_time > time) {
> > +			time = request->mode->modi_time;
> > +			timing_mode = 2;
> > +		}
> > +	}
> 
> [Hardening] Perhaps the case where request->flags has not bits set 
> should be treated with an error returned ?
No need to return error. In this function, we just check these two methods. Others will be processed in other function. If no flag, that is fine, no operation is needed.
> 
> > +
> > +	switch (timing_mode) {
> > +	case 1:
> > +		i = 0;
> > +		wl_list_for_each(mode, &output->mode_list, link) {
> > +			i++;
> > +			if (i == request->modenum.num) {
> > +				found = 1;
> > +				break;
> > +			}
> > +		}
> > +
> > +		if (found != 1)
> > +			mode = NULL;
> > +		break;
> > +	case 2:
> > +		mode = randr_find_mode(output,
> > +				       request->mode->width,
> > +				       request->mode->height,
> > +				       request->mode->refresh);
> > +		break;
> > +	default:
> > +		return;
> > +	}
> 
> [Hardening] related to my remark above, *results should be set in the 
> "default:" handling, as it catches the case where timing_mode == 0 
> which means no flags where set.
If timing_mode is 0, that means, no operation need to be done for switch mode. Just return is enough.
> 
> > +
> > +
> > +	if (!mode) {
> > +		result = WRANDR_TYPE_RET_FAIL;
> > +	} else {
> > +		if (!(mode->flags & WL_OUTPUT_MODE_CURRENT) &&
> > +		    output->switch_mode) {
> > +			result = output->switch_mode(output, mode);
> > +			if (result < 0)
> > +				result = WRANDR_TYPE_RET_FAIL;
> > +			else
> > +				result = WRANDR_TYPE_RET_SUCCESS;
> > +		} else
> > +			result = WRANDR_TYPE_RET_NOACT;
> > +	}
> > +
> > +	move_bits = WRANDR_TYPE_OOP_MODENUM + timing_mode - 1;
> > +	*results &= ~(RESULT_MASK<<(move_bits * 2));
> > +	*results |= result<<(move_bits * 2); }
> > +
> >
> 
> [...]
> 
> Regards
>