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

[Uri Lublin]

On 03/23/2018 01:24 PM, Victor Toso wrote:
> 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.

Thanks, I'll take it.

> 
>> ---
>>   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.

Yes, I'll add them.


Thanks,
     Uri.

> 
> 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