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

[Rémi Bernon]

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.

>>
>>
>> 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.
-- 
Rémi Bernon <rbernon at codeweavers.com>