[PATCH xwayland 2/2] xwayland: check if creating output succeeded

[Marek Chalupa]

Hi, Olivier

On 11/23/2015 02:31 PM, Olivier Fourdan wrote:
> Hi Marek,
>
> ----- Original Message -----
>> check return values of RR.*Create calls and
>> check if we added any output
>>
>> Signed-off-by: Marek Chalupa <mchqwerty at gmail.com>
>> ---
>>   hw/xwayland/xwayland-output.c | 23 +++++++++++++++++++++++
>>   hw/xwayland/xwayland.c        | 10 ++++++++--
>>   2 files changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
>> index 5ef444d..178105c 100644
>> --- a/hw/xwayland/xwayland-output.c
>> +++ b/hw/xwayland/xwayland-output.c
>> @@ -248,6 +248,11 @@ xwl_output_create(struct xwl_screen *xwl_screen,
>> uint32_t id)
>>
>>       xwl_output->output = wl_registry_bind(xwl_screen->registry, id,
>>                                             &wl_output_interface, 2);
>> +    if (!xwl_output->output) {
>> +        ErrorF("Failed binding wl_output\n");
>> +        goto err;
>> +    }
>> +
>>       xwl_output->server_output_id = id;
>>       wl_output_add_listener(xwl_output->output, &output_listener,
>>       xwl_output);
>>
>> @@ -255,13 +260,31 @@ xwl_output_create(struct xwl_screen *xwl_screen,
>> uint32_t id)
>>
>>       xwl_output->xwl_screen = xwl_screen;
>>       xwl_output->randr_crtc = RRCrtcCreate(xwl_screen->screen, xwl_output);
>> +    if (!xwl_output->randr_crtc) {
>> +        ErrorF("Failed creating RandR CRTC\n");
>> +        goto err;
>> +    }
>> +
>>       xwl_output->randr_output = RROutputCreate(xwl_screen->screen, name,
>>                                                 strlen(name), xwl_output);
>> +    if (!xwl_output->randr_output) {
>> +        ErrorF("Failed creating RandR Output\n");
>> +        goto err;
>> +    }
>> +
>>       RRCrtcGammaSetSize(xwl_output->randr_crtc, 256);
>>       RROutputSetCrtcs(xwl_output->randr_output, &xwl_output->randr_crtc, 1);
>>       RROutputSetConnection(xwl_output->randr_output, RR_Connected);
>>
>>       return xwl_output;
>> +
>> +err:
>> +    if (xwl_output->randr_crtc)
>> +        RRCrtcDestroy(xwl_output->randr_crtc);
>> +    if (xwl_output->output)
>> +        wl_output_destroy(xwl_output->output);
>> +    free(xwl_output);
>> +    return NULL;
>>   }
>>
>>   void
>> diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
>> index 56b03f6..2d57826 100644
>> --- a/hw/xwayland/xwayland.c
>> +++ b/hw/xwayland/xwayland.c
>> @@ -397,8 +397,8 @@ registry_global(void *data, struct wl_registry *registry,
>> uint32_t id,
>>               wl_registry_bind(registry, id, &wl_shell_interface, 1);
>>       }
>>       else if (strcmp(interface, "wl_output") == 0 && version >= 2) {
>> -        xwl_output_create(xwl_screen, id);
>> -        xwl_screen->expecting_event++;
>> +        if (xwl_output_create(xwl_screen, id))
>> +            xwl_screen->expecting_event++;
>>       }
>>   #ifdef GLAMOR_HAS_GBM
>>       else if (xwl_screen->glamor &&
>> @@ -599,6 +599,12 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char
>> **argv)
>>       while (xwl_screen->expecting_event > 0)
>>           wl_display_roundtrip(xwl_screen->display);
>>
>> +    /* check if we have any output */
>> +    if (xorg_list_is_empty(&xwl_screen->output_list)) {
>> +        ErrorF("xwayland: Do not have any output");
>> +        return FALSE;
>> +    }
>> +
>
> Do we really have to fail if there is no output?
>
> Would it be possible that a Wayland compositor starts Xwayland before creating/adding the outputs?

I don't think any compositor does so, but AFAIK it is not stated 
anywhere, so it is probably allowed to start Xwayland before creating 
the outputs. Quickly looked into the code that follows this and it 
should work fine with no outputs. So I assume it's ok to run with no 
outputs at this phase. I'll fix the patch.

>
> And what if all outputs get "hot-unplugged" later, should we bail out as well?

I don't think so. If you're changing a monitor cable, you don't suppose 
the DE will give up functioning during that operation. But on hot-unplug 
we don't check the number of outputs, so Xwayland should already behave 
this way.

Thanks,
Marek

>
>>       bpc = xwl_screen->depth / 3;
>>       green_bpc = xwl_screen->depth - 2 * bpc;
>>       blue_mask = (1 << bpc) - 1;
>
> Cheers,
> Olivier
>