[Spice-devel] [PATCH linux vdagent v4 5/9] Factor a function out of get_current_mon_config()

[Lukáš Hrázký]

On Thu, 2019-01-17 at 16:14 -0600, Jonathon Jongsma wrote:
> When sending the guest xorg resolution to the vdagentd daemon, we get
> the current resolution with this function, which returns a
> VDAgentMonitorsConfig structure that must then be converted to the
> structure that we send to the daemon. Rather than re-using this function
> that returns the wrong type, factor out most of the functionality into a
> separate function. This function can be used by a new function to return
> a type appropriate for sending the xorg resolution to the daemon.
> 
> This is necessary because in the next few commits I'll be changing the
> vdagentd protocol to send an additional display_id parameter to the
> daemon.
> ---
>  src/vdagent/x11-randr.c | 73 +++++++++++++++++++++++++++++------------
>  1 file changed, 52 insertions(+), 21 deletions(-)
> 
> diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
> index 5eff225..05a001e 100644
> --- a/src/vdagent/x11-randr.c
> +++ b/src/vdagent/x11-randr.c
> @@ -688,38 +688,69 @@ static int config_size(int num_of_monitors)
>                             num_of_monitors * sizeof(VDAgentMonConfig);
>  }
>  
> +
> +// gets monitor information about the specified output index and returns true if there was no error
> +static bool get_monitor_info_for_output_index(struct vdagent_x11 *x11, int output_index, int *x, int *y, int *width, int *height)

Lines above too long.

> +{
> +    g_return_val_if_fail (output_index < x11->randr.res->noutput, false);
> +    g_return_val_if_fail (x != NULL, false);
> +    g_return_val_if_fail (y != NULL, false);
> +    g_return_val_if_fail (width != NULL, false);
> +    g_return_val_if_fail (height != NULL, false);
> +
> +    int j;
> +    XRRCrtcInfo *crtc = NULL;
> +    XRRModeInfo *mode;
> +
> +    if (x11->randr.outputs[output_index]->ncrtc == 0)
> +        goto zeroed; /* Monitor disabled */
> +
> +    for (j = 0; crtc == NULL && j < x11->randr.outputs[output_index]->ncrtc; j++) {
> +        crtc = crtc_from_id(x11, x11->randr.outputs[output_index]->crtcs[j]);
> +    }
> +    if (!crtc) {
> +        // error. stale xrandr info?
> +        return false;
> +    }
> +
> +    mode = mode_from_id(x11, crtc->mode);
> +    if (!mode)
> +        goto zeroed; /* monitor disabled */
> +
> +    *x = crtc->x;
> +    *y = crtc->y;
> +    *width = mode->width;
> +    *height = mode->height;
> +    return true;
> +
> +zeroed:
> +    *x = 0;
> +    *y = 0;
> +    *width = 0;
> +    *height = 0;
> +    return true;
> +}
> +
>  static VDAgentMonitorsConfig *get_current_mon_config(struct vdagent_x11 *x11)
>  {
>      int i, num_of_monitors = 0;
> -    XRRModeInfo *mode;
>      XRRScreenResources *res = x11->randr.res;
>      VDAgentMonitorsConfig *mon_config;
>  
>      mon_config = g_malloc0(config_size(res->noutput));
>  
>      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; crtc == NULL && j < x11->randr.outputs[i]->ncrtc; j++) {
> -            crtc = crtc_from_id(x11, x11->randr.outputs[i]->crtcs[j]);
> -        }
> -        if (!crtc)
> +        int x, y, width, height;
> +        if (!get_monitor_info_for_output_index(x11, i, &x, &y, &width, &height)) {
> +            syslog(LOG_WARNING, "Unable to get monitor info for output id %d", i);
>              goto error;
> +        }
>  
> -        mode = mode_from_id(x11, crtc->mode);
> -        if (!mode)
> -            continue; /* Monitor disabled, already zero-ed by calloc */
> -
> -        mon_config->monitors[i].x      = crtc->x;
> -        mon_config->monitors[i].y      = crtc->y;
> -        mon_config->monitors[i].width  = mode->width;
> -        mon_config->monitors[i].height = mode->height;
> +        VDAgentMonConfig *mon = &mon_config->monitors[i];
> +        mon->x = x;
> +        mon->y = y;
> +        mon->width = width;
> +        mon->height = height;
>          num_of_monitors = i + 1;
>      }
>      mon_config->num_of_monitors = num_of_monitors;

Reviewed-by: Lukáš Hrázký <lhrazky at redhat.com>