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

[Zhiyi Zhang]

On 5/31/21 6:48 PM, Rémi Bernon wrote:
> On 5/31/21 11:16 AM, Zhiyi Zhang wrote:
>>
>> 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.
>>
>
> Okay, maybe I'm wrong, but as far as I could see, to report fake display devices, monitors and modes, I basically had to reproduce most of winex11.drv / winemac.drv code to user32, which feels even more of a waste.
>
> It may be possible to do it with less, I didn't spend much time on it.

It depends on how much you need to fake. If you just need to get some application to think there is a display, EnumDisplayDevices and EnumDisplayMonitors
should be enough. In fact, wine already reports a fallback display device and a monitor in EnumDisplayDevices and EnumDisplayMonitors.


>
>>>
>>>
>>> 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.
>>
>
> Sure, but IMHO the server doesn't even have to care about this string length, the name could just be considered as a 32 WCHAR fixed size array, and its zero-terminated nature would be handled on the client side.
>
>>>
>>>>    +/* 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.
>>
>
> Sure, but the first failure can just be a return as there's not cleanup to do, in which case the error code path will never be taken with monitor == 0, making the if there unnecessary.
>
>>>> +
>>>> +    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.
>>
>
> Okay, but it's saving at most 62 bytes for each monitor, which doesn't seem very useful. Even then you could do a single allocation of the struct size and the string length, simplifying the error handling.