June 2022 - Wine-Devel - list.winehq.org

Re: [PATCH 0/4] MR233: win32u: Partially move raw input APIs.

by Rémi Bernon

On Wed Jun 15 23:43:22 2022 +0000, **** wrote: > Zebediah Figura replied on the mailing list: > ``` > On 6/14/22 00:02, Rémi Bernon (@rbernon) wrote: > > On Tue Jun 14 04:05:11 2022 +0000, **** wrote: > >> Zebediah Figura replied on the mailing list: > >> \`\`\` > >> On 6/13/22 03:27, Rémi Bernon (@rbernon) wrote: > >>> Rémi Bernon (@rbernon) commented about dlls/user32/tests/input.c: > >>>> + ok(count == ~0u, "GetRawInputData returned %d\n", count); > >>>> + ok(GetLastError() == ERROR_INVALID_PARAMETER, > >> "GetRawInputData returned %08lx\n", GetLastError()); > >>>> + } > >>>> + else if (is_wow64) > >>>> + { > >>>> + count = GetRawInputData((HRAWINPUT)lparam, > >> RID_INPUT, &ri, &size, sizeof(RAWINPUTHEADER64)); > >>>> + todo_wine ok(count == sizeof(ri), "GetRawInputData > >> returned %d\n", count); > >>>> + ok(ri.data.mouse.lLastX == 6, "Unexpected rawinput > >> data: %ld\n", ri.data.mouse.lLastX); > >>>> + todo_wine ok(GetLastError() == 0xdeadbeef, > >> "GetRawInputData returned %08lx\n", GetLastError()); > >>>> + } > >>>> + else > >>>> + { > >>>> + count = GetRawInputData((HRAWINPUT)lparam, > >> RID_INPUT, &ri, &size, sizeof(RAWINPUTHEADER64)); > >>>> + ok(count == ~0u, "GetRawInputData returned %d\n", count); > >>>> + ok(GetLastError() == ERROR_INVALID_PARAMETER, > >> "GetRawInputData returned %08lx\n", GetLastError()); > >>>> + } > >>> It feels a bit unbalanced to test invalid cases at the same time as a > >> valid case depending on the arch. Could we have instead a test for the > >> invalid sizes, and the another one with valid size? > >> I don't understand what you mean by this. Only one of these three tests > >> makes sense, and on mutually exclusive architectures. > >> I could separate the if/elif/else into three separate if blocks, but I > >> doubt that's what you mean. > >> \`\`\` > > I mean that, like the `GetRawInputBuffer` tests below you check an > invalid size and ERROR_INVALID_PARAMETER error case, but on wow64, you > don't and instead you check a valid size and the returned data. > > > > I think you should check the same cases on all arch if possible, so > invalid size and error on all arch (similarly to `GetRawInputBuffer`), > then the valid case and data on all arch as well. > > > The point is to check the "wrong" architecture's size in all three > cases. In the wow64 case, this succeeds; otherwise, it fails. This quirk > is specific to GetRawInputData(); GetRawInputBuffer() always rejects the > "wrong" architecture's size. > Perhaps this structure would make it clearer: > if (sizeof(void *) == 8) > { > count = GetRawInputData(..., sizeof(RAWINPUTHEADER32)); > ok(count == ~0u, ...); > } > else > { > count = GetRawInputData(..., sizeof(RAWINPUTHEADER64)); > if (is_wow64) > { > ok(count == sizeof(ri), ...); > } > else > { > ok(count == ~0u, ...); > } > } > ``` Yeah looks clearer. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/233#note_2178

June 16, 2022

Re: [PATCH 0/5] MR242: winegstreamer: Use wg_sample to read wg_parser data.

by Rémi Bernon

> > diff --git a/dlls/winegstreamer/wg_sample.c b/dlls/winegstreamer/wg_sample.c > > index ae8a2d4d0c7..eb46982fabf 100644 > > --- a/dlls/winegstreamer/wg_sample.c > > +++ b/dlls/winegstreamer/wg_sample.c > > @@ -47,6 +47,11 @@ struct sample > > > > union > > { > > + struct > > + { > > + void *__pad[3]; > > + BYTE buffer[]; > > + } raw; > > struct > > { > > IMFSample *sample; > > Why the padding? So that buffer is aligned to the end of the structure and so that we can safely use a flexible array member. Otherwise offsetof buffer[size] may be shorter than the struct, ending up with a undefined behavior when accessing a partially allocated structure and an eventual GCC warning. > > +HRESULT wg_sample_create_raw(UINT32 size, struct wg_sample **out) > > +{ > > + struct sample *sample; > > + > > + if (!(sample = calloc(1, offsetof(struct sample, u.raw.buffer[size])))) > > + return E_OUTOFMEMORY; > > + > > + sample->wg_sample.data = sample->u.raw.buffer; > > + sample->wg_sample.size = 0; > > + sample->wg_sample.max_size = size; > > + sample->ops = &raw_sample_ops; > > + > > + TRACE("Created wg_sample %p, size %u.\n", &sample->wg_sample, size); > > + *out = &sample->wg_sample; > > + return S_OK; > > +} > > It strikes me as unnecessary to return HRESULT from this function, > especially considering you're not actually using the HRESULT value (only > testing for failure). It'd be simpler just to return "sample" in that case. It makes it consistent with the other sample creation functions, and other creations functions in general. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/242#note_2177

June 16, 2022

Re: [PATCH 0/5] MR242: winegstreamer: Use wg_sample to read wg_parser data.

by Rémi Bernon

On Thu Jun 16 00:25:27 2022 +0000, **** wrote: > Zebediah Figura replied on the mailing list: > ``` > On 6/14/22 02:26, Rémi Bernon wrote: > > From: Rémi Bernon <rbernon(a)codeweavers.com> > > > > It is invalid according to GStreamer documentation. > What documentation are you referring to? > The documentation for GstPadGetRangeFunction itself is somewhat badly > written and doesn't actually mention what "buffer" is expected to > contain on input, but the documentation for gst_pad_get_range() says: > If buffer points to a variable holding NULL, a valid new GstBuffer > will be placed in buffer when this function returns GST_FLOW_OK. The new > buffer must be freed with gst_buffer_unref after usage. > When buffer points to a variable that points to a valid GstBuffer, > the buffer will be filled with the result data when this function > returns GST_FLOW_OK. If the provided buffer is larger than size, only > size bytes will be filled in the result buffer and its size will be > updated accordingly. > and the implementation shows that the intent is for the same to apply to > GstPadGetRangeFunction. > For what it's worth, I believe oggdemux actually does pass a non-NULL > buffer, and breaks if we don't fill that. > ``` I was referring to `If this function returns GST_FLOW_OK, the result buffer will be stored in buffer. The contents of buffer is invalid for any other return value.`, but looking at `gst_pad_get_range` and `gst_pad_pull_range` source, I believe you're right... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/242#note_2176

June 16, 2022

Wiki guidance on taking function prototypes from ReactOS

by Alex Henrie

Yesterday I came across a bug report [1] with an attached patch to stub out the function RtlGetFullPathName_UEx, which is not documented by Microsoft, based on its prototype in ReactOS. Alexandre left the comment: [2] "You should not look at ReactOS code at all, a lot of it is reverse-engineered using methods that are not appropriate for Wine. It's not a usable source of information for us." The patch author, Ben, replied: [3] "I just want the WIKI to be MUCH more definitive/precise on what IS and IS NOT allowed." I took a look and as it currently stands, the wiki clearly encourages collaboration with ReactOS in at least one place: [4] "If you're still resisting the switch to unix for some reason though, you may have some luck investigating ReactOS (they cooperate with us on API issues too)." And it clearly discourages collaboration with ReactOS in at least one place: [5] "it appears that all such documentation is derived from ReactOS, which is not considered a legally safe source for use in Wine." Mention of ReactOS is conspicuously absent from the page that outlines Wine's "clean room" guidelines. [6] Since Alexandre says that Wine developers shouldn't look at ReactOS code at all, I think we should remove from the wiki all references that might be construed to encourage it and add some more that discourage it. Does anyone have any further thoughts before I start editing? -Alex [1] https://bugs.winehq.org/show_bug.cgi?id=50464 [2] https://bugs.winehq.org/show_bug.cgi?id=50464#c6 [3] https://bugs.winehq.org/show_bug.cgi?id=50464#c7 [4] https://wiki.winehq.org/Cygwin_and_More [5] https://wiki.winehq.org/AFD [6] https://wiki.winehq.org/Clean_Room_Guidelines

June 16, 2022

Re: [PATCH v4 0/1] MR134: ntdll: Don't hard-code the battery and AC adapter names on Linux

by Alex Henrie (＠alexhenrie)

On Thu Jun 16 02:37:00 2022 +0000, Alex Henrie wrote: > changed this line in [version 4 of the diff](/wine/wine/-/merge_requests/134/diffs?diff_id=2723&start_sha=d0164f5a44d020b0a50a18fa2ffa46f192c5c5fd#d5bcbaed4ae76fff7d2641017921e07798a7da0e_3418_3418) See above -- https://gitlab.winehq.org/wine/wine/-/merge_requests/134#note_2175

June 16, 2022

Re: [PATCH v4 0/1] MR134: ntdll: Don't hard-code the battery and AC adapter names on Linux

by Alex Henrie (＠alexhenrie)

On Wed Jun 15 19:57:52 2022 +0000, Alexandre Julliard wrote: > snprintf doesn't return -1 on overflow (which is the only failure you > care about here). Thanks for pointing that out. I have corrected the overflow checks, and I will be careful to avoid similar mistakes in the future. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/134#note_2171

June 16, 2022

[PATCH v4 1/1] ntdll: Don't hard-code the battery and AC adapter names on Linux

by Alex Henrie

From: Alex Henrie <alexhenrie24(a)gmail.com> Look through all of the devices in /sys/class/power_supply and take the statistics from the first battery and the first AC adapter. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52831 Signed-off-by: Alex Henrie <alexhenrie24(a)gmail.com> --- dlls/ntdll/unix/system.c | 79 +++++++++++++++++++++++++++------------- 1 file changed, 54 insertions(+), 25 deletions(-) diff --git a/dlls/ntdll/unix/system.c b/dlls/ntdll/unix/system.c index 87cc8b9c3a4..0df20aff3d5 100644 --- a/dlls/ntdll/unix/system.c +++ b/dlls/ntdll/unix/system.c @@ -35,6 +35,7 @@ #include <errno.h> #include <sys/time.h> #include <time.h> +#include <dirent.h> #ifdef HAVE_SYS_PARAM_H # include <sys/param.h> #endif @@ -3379,12 +3380,14 @@ static ULONG mhz_from_cpuinfo(void) return cmz; } -static const char * get_sys_str(const char *path, char *s) +static const char * get_sys_str(const char *dirname, const char *basename, char *s) { - FILE *f = fopen(path, "r"); + char path[64]; + FILE *f; const char *ret = NULL; - if (f) + if (snprintf(path, sizeof(path), "%s/%s", dirname, basename) >= sizeof(path)) return NULL; + if ((f = fopen(path, "r"))) { if (fgets(s, 16, f)) ret = s; fclose(f); @@ -3392,42 +3395,68 @@ static const char * get_sys_str(const char *path, char *s) return ret; } -static int get_sys_int(const char *path, int def) +static int get_sys_int(const char *dirname, const char *basename) { char s[16]; - return get_sys_str(path, s) ? atoi(s) : def; + return get_sys_str(dirname, basename, s) ? atoi(s) : 0; } static NTSTATUS fill_battery_state( SYSTEM_BATTERY_STATE *bs ) { + DIR *d = opendir("/sys/class/power_supply"); + struct dirent *de; char s[16], path[64]; - unsigned int i = 0; + BOOL found_ac = FALSE; LONG64 voltage; /* microvolts */ - bs->AcOnLine = get_sys_int("/sys/class/power_supply/AC/online", 1); + bs->AcOnLine = TRUE; + if (!d) return STATUS_SUCCESS; - for (;;) + while ((de = readdir(d))) { - sprintf(path, "/sys/class/power_supply/BAT%u/status", i); - if (!get_sys_str(path, s)) break; - bs->Charging |= (strcmp(s, "Charging\n") == 0); - bs->Discharging |= (strcmp(s, "Discharging\n") == 0); - bs->BatteryPresent = TRUE; - i++; - } + if (strcmp(de->d_name, ".") == 0 || strcmp(de->d_name, "..") == 0) continue; + if (snprintf(path, sizeof(path), "/sys/class/power_supply/%s", de->d_name) >= sizeof(path)) continue; + if (get_sys_str(path, "scope", s) && strcmp(s, "Device\n") == 0) continue; + if (!get_sys_str(path, "type", s)) continue; - if (bs->BatteryPresent) - { - voltage = get_sys_int("/sys/class/power_supply/BAT0/voltage_now", 0); - bs->MaxCapacity = get_sys_int("/sys/class/power_supply/BAT0/charge_full", 0) * voltage / 1e9; - bs->RemainingCapacity = get_sys_int("/sys/class/power_supply/BAT0/charge_now", 0) * voltage / 1e9; - bs->Rate = -get_sys_int("/sys/class/power_supply/BAT0/current_now", 0) * voltage / 1e9; - if (!bs->Charging && (LONG)bs->Rate < 0) - bs->EstimatedTime = 3600 * bs->RemainingCapacity / -(LONG)bs->Rate; - else - bs->EstimatedTime = ~0u; + if (strcmp(s, "Mains\n") == 0) + { + if (!get_sys_str(path, "online", s)) continue; + if (found_ac) + { + FIXME("Multiple mains found, only reporting on the first\n"); + } + else + { + bs->AcOnLine = atoi(s); + found_ac = TRUE; + } + } + else if (strcmp(s, "Battery\n") == 0) + { + if (!get_sys_str(path, "status", s)) continue; + if (bs->BatteryPresent) + { + FIXME("Multiple batteries found, only reporting on the first\n"); + } + else + { + bs->Charging = (strcmp(s, "Charging\n") == 0); + bs->Discharging = (strcmp(s, "Discharging\n") == 0); + bs->BatteryPresent = TRUE; + voltage = get_sys_int(path, "voltage_now"); + bs->MaxCapacity = get_sys_int(path, "charge_full") * voltage / 1e9; + bs->RemainingCapacity = get_sys_int(path, "charge_now") * voltage / 1e9; + bs->Rate = -get_sys_int(path, "current_now") * voltage / 1e9; + if (!bs->Charging && (LONG)bs->Rate < 0) + bs->EstimatedTime = 3600 * bs->RemainingCapacity / -(LONG)bs->Rate; + else + bs->EstimatedTime = ~0u; + } + } } + closedir(d); return STATUS_SUCCESS; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/134

June 16, 2022

[PATCH v4 0/1] MR134: ntdll: Don't hard-code the battery and AC adapter names on Linux

by Alex Henrie (＠alexhenrie)

Look through all of the devices in /sys/class/power_supply and take the statistics from the first battery and the first AC adapter. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52831 Signed-off-by: Alex Henrie <alexhenrie24(a)gmail.com> -- v4: ntdll: Don't hard-code the battery and AC adapter names on Linux https://gitlab.winehq.org/wine/wine/-/merge_requests/134

June 16, 2022

Re: [PATCH 5/5] winegstreamer: Use wg_sample to provide data in wg_parser_reply_read.

by Zebediah Figura

On 6/14/22 02:27, Rémi Bernon wrote: > diff --git a/dlls/winegstreamer/wg_sample.c b/dlls/winegstreamer/wg_sample.c > index ae8a2d4d0c7..eb46982fabf 100644 > --- a/dlls/winegstreamer/wg_sample.c > +++ b/dlls/winegstreamer/wg_sample.c > @@ -47,6 +47,11 @@ struct sample > > union > { > + struct > + { > + void *__pad[3]; > + BYTE buffer[]; > + } raw; > struct > { > IMFSample *sample; Why the padding? > +HRESULT wg_sample_create_raw(UINT32 size, struct wg_sample **out) > +{ > + struct sample *sample; > + > + if (!(sample = calloc(1, offsetof(struct sample, u.raw.buffer[size])))) > + return E_OUTOFMEMORY; > + > + sample->wg_sample.data = sample->u.raw.buffer; > + sample->wg_sample.size = 0; > + sample->wg_sample.max_size = size; > + sample->ops = &raw_sample_ops; > + > + TRACE("Created wg_sample %p, size %u.\n", &sample->wg_sample, size); > + *out = &sample->wg_sample; > + return S_OK; > +} It strikes me as unnecessary to return HRESULT from this function, especially considering you're not actually using the HRESULT value (only testing for failure). It'd be simpler just to return "sample" in that case.

June 16, 2022

Re: [PATCH 4/5] winegstreamer: Rename wg_parser_get_next_read_offset to wg_parser_wait_request.

by Zebediah Figura

On 6/14/22 02:27, Rémi Bernon wrote: > static void handle_read_request(struct media_source *source, QWORD file_size, > - void **buffer, size_t *buffer_size, uint64_t offset, uint32_t size) > + void **buffer, size_t *buffer_size, struct wg_request *request) This can be constified, right? > diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h > index 860a8ab2a52..f767daed513 100644 > --- a/dlls/winegstreamer/unixlib.h > +++ b/dlls/winegstreamer/unixlib.h > @@ -135,6 +135,24 @@ struct wg_sample > BYTE *data; > }; > > +struct wg_request > +{ > + enum wg_request_type > + { > + WG_REQUEST_NONE = 0, > + WG_REQUEST_READ = 1, > + } type; > + > + union > + { > + struct > + { > + UINT32 size; > + UINT64 offset; > + } read; > + } u; > +}; > + > struct wg_parser_buffer > { > /* pts and duration are in 100-nanosecond units. */ What's the purpose of WG_REQUEST_NONE?

June 16, 2022