[Spice-devel] [spice-vdagnet (linux) PATCH] x11-randr: do not assume each output has ncrtc=1

[Victor Toso]

Hi,

On Tue, Mar 20, 2018 at 05:24:42PM +0200, Uri Lublin wrote:
> This was true for virtual graphic cards, but not true for
> device-assigned graphic cards.
> For example NVIDIA M2000 has 8.
> 
> Still we currently pick only a single crtc for each output
> (but looping over them to find an active one)
> 
> Signed-off-by: Uri Lublin <uril at redhat.com>

I got a [-Werror=maybe-uninitialized] here:

src/vdagent/x11-randr.c: In function ‘get_current_mon_config’:
src/vdagent/x11-randr.c:686:46: error: ‘crtc’ may be used uninitialized in this function
         mon_config->monitors[i].x      = crtc->x;
                                          ~~~~^~~

Quite weird, it does not realize that we checked ncrtc != 0
before, which means that crtc_from_id() will be called at least
once!

A suggestion patch to squash is attached with the suggestions I'm
giving inline.

> ---
>  src/vdagent/x11-randr.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
> index aade5ca..0c196de 100644
> --- a/src/vdagent/x11-randr.c
> +++ b/src/vdagent/x11-randr.c
> @@ -58,6 +58,8 @@ static XRRCrtcInfo *crtc_from_id(struct vdagent_x11 *x11, int id)
>  {
>      int i;
>  
> +    if (id == 0)
> +        return NULL;

Brackets! Not that important but I get that for new code we
should try to use brackets.

Out of curiosity:
* Number of if without brackets: 600
  # grep -rniI "if.*(.*" * | grep * -v "{" | wc
* Number of if with brackets: 409
  $ grep -rniI "if.*(.*{" * | wc

So, without brackets is actually majority in vd_agent :)

>      for (i = 0 ; i < x11->randr.res->ncrtc ; ++i) {
>          if (id == x11->randr.res->crtcs[i]) {
>              return x11->randr.crtcs[i];
> @@ -650,7 +652,7 @@ static int config_size(int num_of_monitors)
>  
>  static VDAgentMonitorsConfig *get_current_mon_config(struct vdagent_x11 *x11)
>  {
> -    int i, num_of_monitors = 0;
> +    int i, j, num_of_monitors = 0;

I would move both j and *crtc inside the for block and initialize
crtc to NULL to avoid the warning.

>      XRRModeInfo *mode;
>      XRRCrtcInfo *crtc;
>      XRRScreenResources *res = x11->randr.res;
> @@ -665,10 +667,15 @@ static VDAgentMonitorsConfig *get_current_mon_config(struct vdagent_x11 *x11)
>      for (i = 0 ; i < res->noutput; i++) {
>          if (x11->randr.outputs[i]->ncrtc == 0)
>              continue; /* Monitor disabled, already zero-ed by calloc */
> -        if (x11->randr.outputs[i]->ncrtc != 1)
> -            goto error;
> +        if (x11->randr.outputs[i]->crtc == 0)
> +            continue; /* Monitor disabled */
>  
> -        crtc = crtc_from_id(x11, x11->randr.outputs[i]->crtcs[0]);
> +        for (j=0; j<x11->randr.outputs[i]->ncrtc; j++) {
> +            crtc = crtc_from_id(x11, x11->randr.outputs[i]->crtcs[j]);
> +            if (crtc) {
> +                break;
> +            }

You can add the crtc check in the for (...; crtc != NULL && ;...)
as well.

Reviewed-by: Victor Toso <victortoso at redhat.com>

> +        }
>          if (!crtc)
>              goto error;
>  
> -- 
> 2.14.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
From 4886daa72a3ddf1b42ec26a8b25a135569db9db3 Mon Sep 17 00:00:00 2001
From: Victor Toso <me at victortoso.com>
Date: Fri, 23 Mar 2018 11:23:29 +0100
Subject: [PATCH] squash-me

---
 src/vdagent/x11-randr.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
index 0c196de..803cf73 100644
--- a/src/vdagent/x11-randr.c
+++ b/src/vdagent/x11-randr.c
@@ -58,8 +58,10 @@ static XRRCrtcInfo *crtc_from_id(struct vdagent_x11 *x11, int id)
 {
     int i;
 
-    if (id == 0)
+    if (id == 0) {
         return NULL;
+    }
+
     for (i = 0 ; i < x11->randr.res->ncrtc ; ++i) {
         if (id == x11->randr.res->crtcs[i]) {
             return x11->randr.crtcs[i];
@@ -652,9 +654,8 @@ static int config_size(int num_of_monitors)
 
 static VDAgentMonitorsConfig *get_current_mon_config(struct vdagent_x11 *x11)
 {
-    int i, j, num_of_monitors = 0;
+    int i, num_of_monitors = 0;
     XRRModeInfo *mode;
-    XRRCrtcInfo *crtc;
     XRRScreenResources *res = x11->randr.res;
     VDAgentMonitorsConfig *mon_config;
 
@@ -665,16 +666,16 @@ static VDAgentMonitorsConfig *get_current_mon_config(struct vdagent_x11 *x11)
     }
 
     for (i = 0 ; i < res->noutput; i++) {
+        int j;
+        XRRCrtcInfo *crtc = NULL;
+
         if (x11->randr.outputs[i]->ncrtc == 0)
             continue; /* Monitor disabled, already zero-ed by calloc */
         if (x11->randr.outputs[i]->crtc == 0)
             continue; /* Monitor disabled */
 
-        for (j=0; j<x11->randr.outputs[i]->ncrtc; j++) {
+        for (j = 0; crtc == NULL && j < x11->randr.outputs[i]->ncrtc; j++) {
             crtc = crtc_from_id(x11, x11->randr.outputs[i]->crtcs[j]);
-            if (crtc) {
-                break;
-            }
         }
         if (!crtc)
             goto error;
-- 
2.16.2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180323/800b0e03/attachment.sig>