[PATCH v4 3/6] winex11.drv: Store monitor information in the wineserver for EnumDisplayMonitors().

[Zhiyi Zhang]

On 5/22/21 1:25 AM, Rémi Bernon wrote:
> Hi Zhiyi!
>
> I had a look, as I was working on the null graphics driver and as the monitor code is the only thing remaining for it to pass user32 tests.
>
>
> IMHO this could at least be split (unless there's something I missed that makes it hard), with first the monitor info being sent to wineserver, then using it for enumeration and then to get monitor info in separate patches.

Hi Rémi,

Yes, I could split the patch that way. But it will then create a patch that does nothing with only the monitor information in the wineserver and they're not used anywhere.
I was advised not to do create such patches before.

>
>
> Then, I think duplicating the code between winex11.drv and winemac.drv is not great, and that it would be nice if this could be done in user32 instead.
>
> It would be a bit more work to refactor the driver interface first, but I think it would be a nice way to reduce code duplication, and it would then be much easier to expose fake monitors and modes as a default implementation, for the null graphics driver.
>
>
> For instance, I think the driver interface should probably expose get_gpus / get_adapters / get_monitors callbacks.
>
> We could even maybe name them pEnumDisplayDevices / pEnumDisplayMonitors and keep it closer to their user32 entry point counterparts, and to make it more consistent with the other driver entry points.
>
> It may only be worth the trouble if we can return the information we need from the drivers in the related user32 structures, as otherwise we'll need custom struct anyway.

I'm aware of the duplication for tje display device initialization code in winex11.drv and winemac.drv. I just don't think the refactoring is worth the trouble at this point. There is also
of the question of whether it is appropriate to do display device enumeration in user32. I think you don't have to refactor the display device interface to refactor the null graphics driver.

>
>
> Then, user32 would call these to enumerate the devices, factoring winex11.drv and winemac.drv init_display_devices into user32.
>
>
> On 5/19/21 10:14 AM, Zhiyi Zhang wrote:
>> @@ -3974,9 +3975,35 @@ fail:
>>   BOOL CDECL nulldrv_GetMonitorInfo( HMONITOR handle, MONITORINFO *info )
>>   {
>>       UINT index = (UINT_PTR)handle - 1;
>> +    NTSTATUS status;
>>         TRACE("(%p, %p)\n", handle, info);
>>   +    SERVER_START_REQ( get_monitor_info )
>> +    {
>> +        req->handle = wine_server_user_handle( handle );
>> +        if (info->cbSize >= sizeof(MONITORINFOEXW))
>
> I think this is enforced to be == (or == sizeof(MONITORINFO)) elsewhere, having >= feels pretty confusing.

Thanks. I will change it to == sizeof(MONITORINFOEXW)

>
>> +            wine_server_set_reply( req, ((MONITORINFOEXW *)info)->szDevice,
>> +                                   sizeof(((MONITORINFOEXW *)info)->szDevice) - sizeof(WCHAR) );
>> +        if (!(status = wine_server_call( req )))
>> +        {
>> +            SetRect( &info->rcMonitor, reply->monitor_rect.left, reply->monitor_rect.top,
>> +                     reply->monitor_rect.right, reply->monitor_rect.bottom );
>> +            SetRect( &info->rcWork, reply->work_rect.left, reply->work_rect.top,
>> +                     reply->work_rect.right, reply->work_rect.bottom );
>> +            if (!info->rcMonitor.left && !info->rcMonitor.top && info->rcMonitor.right && info->rcMonitor.bottom)
>> +                info->dwFlags = MONITORINFOF_PRIMARY;
>> +            else
>> +                info->dwFlags = 0;
>> +            if (info->cbSize >= sizeof(MONITORINFOEXW))
>> +                ((MONITORINFOEXW *)info)->szDevice[wine_server_reply_size( req ) / sizeof(WCHAR)] = 0;
>
> Is this even guaranteed to be zero-terminated? Also see comments below, as it's a fixed 32 WCHAR array, I think we could maybe just pass it around.

Yes, it's zero-terminated. Unless we want to store a '\0' in the wineserver and we don't have to do this. The previous review from Alexandre said
that strings in wineserver shouldn't be zero-terminated.

>
>>   +/* Wine server monitor list management */
>> +
>> +struct wine_server_monitor_info
>> +{
>> +    unsigned int entry_count;
>> +    unsigned int entry_capacity;
>> +    struct update_monitor_entry *entries;
>> +};
>> +
>> +static BOOL wine_server_add_monitor_info(struct wine_server_monitor_info *info,
>> +                                         const struct x11drv_monitor *monitor, int adapter_index)
>
> I'm sure it's better to keep wine_server_ prefixes for the few core functions.
>
>> diff --git a/server/display.c b/server/display.c
>> new file mode 100644
>> index 00000000000..8e8ac2d1338
>> --- /dev/null
>> +++ b/server/display.c
>> @@ -0,0 +1,133 @@
>> +/*
>> + * Server-side display device management
>> + *
>> + * Copyright (C) 2021 Zhiyi Zhang for CodeWeavers
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
>> + */
>> +#include "config.h"
>> +
>> +#include <stdarg.h>
>> +
>> +#include "ntstatus.h"
>> +#define WIN32_NO_STATUS
>> +#include "winternl.h"
>> +
>> +#include "request.h"
>> +#include "user.h"
>> +
>> +static struct list monitor_list = LIST_INIT(monitor_list);
>> +static unsigned int monitor_count;
>> +
>> +/* retrieve a pointer to a monitor from its handle */
>> +static struct monitor *get_monitor( user_handle_t handle )
>> +{
>> +    struct monitor *monitor;
>> +
>> +    if (!(monitor = get_user_object( handle, USER_MONITOR )))
>> +        set_win32_error( ERROR_INVALID_MONITOR_HANDLE );
>> +    return monitor;
>> +}
>> +
>> +/* create a monitor */
>> +static void create_monitor( const struct update_monitor_entry *entry )
>> +{
>> +    struct monitor *monitor;
>> +
>> +    if (!(monitor = mem_alloc( sizeof(*monitor) )))
>> +        goto failed;
>
> This is unnecessary and it actually makes the error path more complicated.
>

I think memory allocation failures should be handled.

>> +
>> +    if (!(monitor->adapter_name = memdup( entry->adapter_name, entry->adapter_name_len )))
>> +        goto failed;
>> +    monitor->adapter_name_len = entry->adapter_name_len;
>> +
>> +    if (!(monitor->handle = alloc_user_handle( monitor, USER_MONITOR )))
>> +        goto failed;
>> +
>> +    monitor->monitor_rect = entry->monitor_rect;
>> +    monitor->work_rect = entry->work_rect;
>> +    list_add_tail( &monitor_list, &monitor->entry );
>> +    ++monitor_count;
>> +    return;
>> +
>> +failed:
>> +    if (monitor)
>> +    {
>> +        if (monitor->adapter_name)
>> +            free( monitor->adapter_name );
>> +        free( monitor );
>> +    }
>> +}
>
> To make things even simpler, I think you should move adapter_name to the end of the struct, as a WCHAR[32], and allocate all memory at once.
>
> Then there would be only one error code path, when the handle allocation fails, and it can simply free the memory and return.

It's done this way to save memory.

>
>> +/* modify the list of monitors */
>> +DECL_HANDLER(update_monitors)
>> +{
>> +    const struct update_monitor_entry *entries;
>> +    struct monitor *monitor, *monitor2;
>> +    unsigned int entry_count, i;
>> +
>> +    LIST_FOR_EACH_ENTRY_SAFE(monitor, monitor2, &monitor_list, struct monitor, entry)
>> +    {
>> +        list_remove( &monitor->entry );
>> +        free_user_handle( monitor->handle );
>> +        free( monitor->adapter_name );
>> +        free( monitor );
>> +    }
>> +    monitor_count = 0;
>> +
>> +    entries = get_req_data();
>> +    entry_count = get_req_data_size() / sizeof(*entries);
>> +    for (i = 0; i < entry_count; ++i)
>> +        create_monitor( &entries[i] );
>> +}
>
> I think this is going to change the handles every time the list is updated, is this going to be an issue?

No. Applications should receive a WM_DISPLAYCHANGE notification message and use the new handles.

Thanks for the review!

Zhiyi

>
>> diff --git a/server/protocol.def b/server/protocol.def
>> index 6d8208b128b..3d536a0a237 100644
>> --- a/server/protocol.def
>> +++ b/server/protocol.def
>> @@ -2693,6 +2693,45 @@ enum coords_relative
>>   #define SET_USER_OBJECT_GET_FULL_NAME   2
>>     +struct update_monitor_entry
>> +{
>> +    rectangle_t    monitor_rect;     /* monitor rectangle */
>> +    rectangle_t    work_rect;        /* monitor work area rectangle */
>> +    WCHAR          adapter_name[32]; /* adapter name */
>> +    data_size_t    adapter_name_len; /* adapter name length in bytes */
>> +};
>
> Do we need the length here? As I understand it, this will only ever be used to fill MONITORINFOEXW structures, which have a fixed size buffer as well.
>
> Could we just pass the fixed size WCHAR array? Although I don't know if it's good practice.
>
>> +
>> +struct enum_monitor_entry
>> +{
>> +    user_handle_t  handle;         /* handle to the monitor */
>> +    rectangle_t    monitor_rect;   /* monitor rectangle */
>> +};
>> +
>> +
>> +/* Modify the list of monitors */
>> + at REQ(update_monitors)
>> +    VARARG(monitors,update_monitor_entry); /* A list of monitors to be created */
>> + at END
>> +
>> +
>> +/* Get information about a monitor */
>> + at REQ(get_monitor_info)
>> +    user_handle_t  handle;         /* handle to the monitor */
>> + at REPLY
>> +    rectangle_t    monitor_rect;   /* monitor rectangle */
>> +    rectangle_t    work_rect;      /* monitor work area rectangle */
>> +    VARARG(adapter,unicode_str);   /* adapter name */
>> + at END
>> +
>> +
>> +/* Enumerate monitors */
>> + at REQ(enum_monitors)
>> + at REPLY
>> +    unsigned int   count;          /* total count of monitors */
>> +    VARARG(monitors,enum_monitor_entry); /* A list of monitors enumerated */
>> + at END
>> +
>> +
>>   /* Register a hotkey */
>>   @REQ(register_hotkey)
>>       user_handle_t  window;        /* handle to the window */
>> diff --git a/server/user.h b/server/user.h
>> index 80f7e91f12c..a9807f1402d 100644
>> --- a/server/user.h
>> +++ b/server/user.h
>> @@ -36,7 +36,8 @@ enum user_object
>>   {
>>       USER_WINDOW = 1,
>>       USER_HOOK,
>> -    USER_CLIENT  /* arbitrary client handle */
>> +    USER_CLIENT,  /* arbitrary client handle */
>> +    USER_MONITOR
>>   };
>>     #define DESKTOP_ATOM  ((atom_t)32769)
>> @@ -79,6 +80,16 @@ struct desktop
>>       unsigned char        keystate[256];    /* asynchronous key state */
>>   };
>>   +struct monitor
>> +{
>> +    user_handle_t        handle;           /* monitor handle */
>> +    struct list          entry;            /* entry in global monitor list */
>> +    rectangle_t          monitor_rect;     /* monitor rectangle */
>> +    rectangle_t          work_rect;        /* monitor work area rectangle */
>> +    WCHAR                *adapter_name;    /* adapter name */
>> +    data_size_t          adapter_name_len; /* adapter name length */
>> +};
>> +
>
> Same comment as above.
>
> Cheers,