Nikolay Sivov wrote:
> +FreeLibrary(mod);
Please add the tests for FreeLibrary return value.
--
Dmitry.
IID_IUnknown) ||
IsEqualGUID(riid, &IID_IAnotherSupported)
{
xxx_AddRef(iface);
*object = iface;
return S_OK;
}
*object = NULL;
return E_NOINTERFACE;
}
--
Dmitry.
ualGUID(riid, &IID_IUnknown))
> {
> -*ret_iface = iface;
> IDirect3DRMVisualArray_AddRef(iface);
> +*out = iface;
> return S_OK;
> }
Although this is existing code the assignment '*out = iface' is wrong,
especially since next patch introduces impl_from_IDirect3DRMVisualArray()
helper. Looks like that file needs a bit of COM clean up.
--
Dmitry.
Dmitry Timoshkov wrote:
> diff --git a/server/file.c b/server/file.c
> index 2ecf97c..94d3060 100644
> --- a/server/file.c
> +++ b/server/file.c
> @@ -459,7 +459,7 @@ static mode_t file_access_to_mode( unsigned int access )
>
> access = generic_file_map_access( access
sn't that convincing enough? If not, what else do you need
to see?
--
Dmitry.
for overlapped writes and reads? But that's clearly
not how Windows implements this accordingto the tests, and that would
require changing all the tests to accept both values so that they pass
under Wine.
--
Dmitry.
rly show that up-to-date Windows versions
(including Windows XP) always return STATUS_PENDING for asynchronous read
and write on disk files. Is there another way to remove all those todo_wine
statements from the tests?
--
Dmitry.
try to figure out why this test doesn't
always pass. I have no no idea what kind of results you have in mind that
I'd need to share. Also I'd appreciate avoiding the wording like "please
don't waste my time", otherwise it becomes pretty obvious that I should
just reply to failure notifications in the testbot with simple "it's broken"
and stop wasting my time explaining why.
--
Dmitry.
t; > > comm.c:1528: Changing CommMask on the fly for handle 00F8 after
> > > timeout 500
> >
> > This VM is broken,
>
> How so? That is: what should I do to fix it?
Investigate, find the source of failures and fix it? Note, that all
other VMs pass this test just fine, moreover, this same w7pro64 VM
doesn't always fail.
--
Dmitry.
Dmitry Timoshkov wrote:
> Marvin wrote:
>
> > === w7pro64 (32 bit comm) ===
> > comm.c:863: Test failed: WaitCommEvent failed with a timeout
> > comm.c:875: recovering after WAIT_TIMEOUT...
> > comm.c:886: Test failed: WaitCommEvent error 0
> > comm.c:888:
This VM is broken, my working computer runs Windows 7 Pro 64-bit on
real hardware, and I run both 32-bit and 64-bit tests with and without
serial devices powered on.
--
Dmitry.
nly in the cases
known to not contain STATUS_PENDING of a being executed operation,
which means that the operation has already finished and waiting would
return immediately and therefore is not necessary and redundant.
--
Dmitry.
bytes == 0, "bytes %u\n", bytes);
> >
> That's again a test for kernel32 calls, placed in a wrong file.
Please actually look at the tests before commenting.
--
Dmitry.
suggest to accept the patch as as, and make further
improvements based on it.
--
Dmitry.
Francois Gouget wrote:
> That tree was a bit out of date causing the patch to fail to apply. I
> updated it and rediffed.
I'd appreciate if you could postpone sending this sort of patches
when they could conflict with other pending patches in that area.
Thanks.
--
Dmitry.
> It doesn't matter what it uses internally. You're testing kernel32 calls.
What are you trying to prove? Half of the calls in that file are using
kernel32 APIs, that doesn't mean that they need to be moved to kernel32
or that they are doing something wrong.
--
Dmitry.
Nikolay Sivov wrote:
> It looks like it belongs to kernel32/tests.
Since actual access checks are done by ntdll APIs I believe that ntdll/tests
is appropriate place, kernel32 file APIs are just wrappers around the tested
functionality.
--
Dmitry.
INPOS_GetMinMaxInfo(), too.
Do you really need anything besides ptMaxSize? In any way even duplicating
some of that code is cleaner than adding one more private export IMHO.
--
Dmitry.
lls directories that may be affected.
--
Dmitry.
out 500 handle 00F8
> comm.c:1528: Changing CommMask on the fly for handle 00F8 after timeout
> 500
These failures have nothing to do with this patch.
--
Dmitry.
Ken Thomases wrote:
> +@ cdecl __wine_get_min_max_info(long ptr ptr ptr ptr) WINPOS_GetMinMaxInfo
What's wrong with calling SendMessage(WM_GETMINMAXINFO) from the driver?
--
Dmitry.
x27;d guess that it should be set in any case since 'events' is
always initialized.
--
Dmitry.
Alexandre Julliard wrote:
> Why do you need that? It doesn't look to me like an improvement.
IMO it simplifies the code and avoids using two different code paths
for normal files.
--
Dmitry.
e caller.
Of course you are correct, thanks.
--
Dmitry.
ut a patch set which does:
1/2. add a workaround to the tests to pass under Wine
2/2. fix Wine code and simultaneously remove the workaround added by 1/2.
But that looks even more strange if the fix contained in 2/2 could be
sent alone.
--
Dmitry.
really related.
Adding a workaround to the tests to compensate a Wine bug and as a side effect
remove some todo_wines is not a fix. Yes, some tests depend on each other, but
that's on purpose, there is no need to break this dependency just because you
can make some later tests suddenly "pass".
--
Dmitry.
; > func=3 method=0) comm.c:2036: Tests skipped: interactive tests (set
> > > WINETEST_INTERACTIVE=1)
> > Successes inside of todo_wine blocks are treated as failres.
>
> So you think I should remove the wine_todos already here?
No, the source of the failures is still there.
--
Dmitry.
gt; No, the source of the failures is still there.
>
> What do you mean with that? The tests indeed do succeed now and there is a
> reason they do: when you call WaitCommEvent() while the tx buffer is not
> empty
> yet the wine code will detect that EV_TXEMPTY correctly:
The tests must pass under Wine without any additional "fixes" as they do
currently under Windows. If you add some code to the tests which suddenly
makes them pass under Wine - that's not a fix, Wine implementation should
be fixed instead.
--
Dmitry.
0 };
This should be 'static const'.
--
Dmitry.
side todo block: WaitCommEvent used 1141 ms for
> waiting
> err:comm:set_line_control ByteSize
> fixme:ntdll:server_ioctl_file Unsupported ioctl 1b000c (device=1b access=0
> func=3 method=0)
> comm.c:2036: Tests skipped: interactive tests (set WINETEST_INTERACTIVE=1)
Successes inside of todo_wine blocks are treated as failres.
--
Dmitry.
sts the EV_TXEMPTY behaviour:
>
> Again 17 bytes are written and then the tests assume that one waits
> for these 17 bytes (timeout value and messurement).
> But really we wait for much more bytes being sent, up to 36.
>
> So even if EV_TXEMPTY handling would work the test for it will fail with a
> timeout.
Does 'make test' pass without failures for you?
--
Dmitry.
gt; >
> > When Wine behaviour differs from Windows one the test results need to be
> > marked as todo_wine, and such places already have it.
>
> I don't unterstand you. I didn't change the test case. It still above and is
> still marked with todo.
There is no need to add any workarounds for Wine bugs, appropriate places
already have todo_wine statements.
--
Dmitry.
gt; incorrect or incomplete.
>
> Then you think that wine behaviour should be changed?
If the test result is marked as todo_wine then yes.
> If yes I'll send a
> patch. But I wonder as wine does quit some effort to do otherwise.
I'm already working on fixing bugs discovered by kernel32 comm and related
ntdll file tests.
--
Dmitry.
the GetLastError function returns ERROR_IO_PENDING,
> indicating that the operation is executing in the background."
We have the tests for that, MSDN descriptions are proved to be often incorrect
or incomplete.
--
Dmitry.
and serial-USB cable
> > under Windows or Linux here and under testbot VMs.
>
> Wine does that here (vanilla). I added this so that the NEXT test does not
> depend what wine exactly does.
When Wine behaviour differs from Windows one the test results need to be
marked as todo_wine, and such places already have it.
--
Dmitry.
at WaitCommEvent returns immediately with TRUE if there is an
> event pending, as far as I can see.
WaitCommEvent always returns FALSE when device is opened in overlapped mode.
--
Dmitry.
y sent 150 bits/s?
>
> Maybe you are using a virtual machine or your com port does not support 150
> baud and chooses a different speed. Or you use a UART with a large buffer and
> which already signals TX-EMPTY even if it is still sending from an internal
> buffer. But one should not rely on that.
I'm testing under 64-bit Windows 7 on real hardware.
--
Dmitry.
GetLastError());
This change looks spurious and unrelated. Also todos must be removed
in the patch that fixes appropriate behaviour.
--
Dmitry.
and under testbot VMs.
--
Dmitry.
s. Testbot VMs
also don't fail this test. If under Wine it takes > 1000 ms for you then
probably there is a bug somewhere.
--
Dmitry.
is no need to invent new keywords for broken behaviour.
--
Dmitry.
fly for handle 0130 after timeout
> 500
These are all existing tests, nothing to do with this patch. Also it's
strange that 1/2 has passed these tests without failures while 2/2 fails.
This new testbot behaviour doesn't look very robust.
--
Dmitry.
, and looked around in various
mailing lists, which made me think that your kernel may set UART_LSR_TEMT
instead...
--
Dmitry.
on't, but maybe yours does. The only way to check that is to store
and print the whole LSR value.
Do you happen to have something connected to COM-port (perhaps mouse?)
which may explain test failures.
--
Dmitry.
Dmitry Timoshkov wrote:
> Printing complete lsr value in the log may help with diagnosing
> test failures on Alexandre's machine.
This patch is marked as 'Build failure' in the tracker, but I've checked it
once again and compiles just fine here. What kind of build failure is that?
--
Dmitry.
Dmitry Timoshkov wrote:
> Based on a patch of Wolfgang Walter.
Is there anything wrong with this patch?
--
Dmitry.
Dmitry Timoshkov wrote:
> Wolfgang Walter wrote:
>
> > I think that happens:
> >
> > * application writes data to com port.
> > * all is written, serial buffer is empty
> > * application calls WaitCommEvent()
> > * wait_on() is called
> > * w
n between:
That means that WaitCommEvent(EV_TXEMPTY) without any prior WriteFile
would always return success and signal the EV_TXEMPTY event. My tests
show that WaitCommEvent(EV_TXEMPTY) fails with a timeout in this case.
Since the serial device is very slow it should be unlikely that after
successful WriteFile() with some data WaitCommEvent() sees an already
empty transmitter's output queue.
--
Dmitry.
>
> The WriteFile error is a timing thing, it doesn't always happen (and
> that test is clearly broken, ticks can be incremented at any point).
> The other errors always happen. It's a real COM port.
Could you please generate a log with additional trace for the empty flag?
--
Dmitry.
Dmitry Timoshkov wrote:
> Alexandre Julliard wrote:
>
> > It doesn't work here:
> >
> > ../../../tools/runtest -q -P wine -M kernel32.dll -T ../../.. -p
> > kernel32_test.exe.so comm.c && touch comm.ok
> > comm.c:835: Test failed: WriteFile t
INETEST_INTERACTIVE=1)
> make: *** [comm.ok] Error 5
I assume it's a real hardware and not a VM? Is this with a real COM port,
or USB-serial cable? If the latter one what driver is it using?
--
Dmitry.
Andrew Cook wrote:
> On 27/08/13 22:02, Dmitry Timoshkov wrote:
> > Andrew Cook wrote:
> >> --- a/include/wine/server_protocol.h
> >> +++ b/include/wine/server_protocol.h
> >
> > And don't include autogenerated stuff in the patch.
> >
>
process );
> +static NTSTATUS (WINAPI *pNtIsProcessInJob)( HANDLE process, HANDLE job );
> +static NTSTATUS (WINAPI *pNtTerminateJobObject)( HANDLE job, NTSTATUS status
> );
Please move it to the beginning of the file.
> --- a/include/wine/server_protocol.h
> +++ b/include/wine/server_protocol.h
And don't include autogenerated stuff in the patch.
--
Dmitry.
which doesn't work? What hardware and driver are you using?
--
Dmitry.
Marvin wrote:
> === WVISTAX64 (32 bit comm) ===
> comm.c:835: Test failed: WriteFile took 16 ms to write 0 Bytes at 150 Baud
Failure is not caused by this patch, probably a VM is very slow.
--
Dmitry.
Alexandre Julliard wrote:
> It's a blocking call, you can't do that on the server side.
Can then something like in the patch from Wolfgang Walter be done
instead?
--
Dmitry.
iles.
There is a test for pipes which fails if there is no this limitation.
--
Dmitry.
some reason ignored
> > [please bottom post when replying]
--
Dmitry.
== 0xc011
> file.c:1008: Test failed: Invalid ioSb.Information: 3
This is an existing test.
Same for other VMs with failures.
--
Dmitry.
help/tests/apphelp.c
> [2] https://github.com/krofna/wine/blob/master/dlls/kernel32/process.c#L1088
How is this supposed to be used in Wine?
--
Dmitry.
from? Is there an application that calls
apphelp APIs and doesn't work because of failures? How are you
testing your implementation?
--
Dmitry.
< 23 &&
> +rect->Width > 0.0 && rect->Height > 0.0)
> {
> /* FIXME: If only the width or only the height is 0, we should
> probably still clip */
> rgn = CreatePolygonRgn(corners, 4, ALTERNATE);
Probably FIXME should be removed then.
--
Dmitry.
r whether it's broken 4.0 headers.
So I don't see how checking for the size of toff_t makes it possible to
differentiate between 3.x, 4.x and broken 4.x headers.
--
Dmitry.
guments not
> > clear enough?
>
> You shouldn't check the version, but the actual problem (i.e. the size
> of the offending type). Also please avoid unnecessary changes.
What changes do you consider as unnecessary?
--
Dmitry.
Dmitry Timoshkov wrote:
> > There's a TIFF_VERSION define that seems to have been renamed to
> > TIFF_VERSION_CLASSIC in 4.0.
> >
> > It doesn't make sense to do the check at runtime, given we're only
> > going to link to one of the versions.
>
erentiate
between 3.x and 4.x at compile time.
--
Dmitry.
tro problem, it's a libtiff bug.
> Actually, this will be a problem on libtiff 3.x, where toff_t really
> is supposed to be 32-bit.
Yes, it will be broken for libtiff3, I haven't looked yet how to detect
a libtiff version at compile and run time, if you know how to do that
please let me know.
--
Dmitry.
Vincent Povirk wrote:
> I'm curious what happens if the world transform has some rotation. It
> seems like transform_rectf would break in that case.
It works just fine in the samples I have here, why would it break?
--
Dmitry.
eaders as another complexity level, then decide whether you really
would like to add another hoop to jump through.
Compilation with a PSDK tool chain and PSDK headers is an extremely valuable
thing for testing, it would be a pity to lose it.
--
Dmitry.
Nikolay Sivov wrote:
> +#include "ddk/wdm.h"
Please don't do that, this breaks compilation with PSDK headers. If you need
some specific definitions, types or API prototypes - replicate them in the test
itself.
--
Dmitry.
ulting palette should be tested and released. I'd guess that newer
version should be taken as a model to follow, and Wine implementation
should be fixed appropriately.
--
Dmitry.
> +static inline DWORD get_tpidrurw(void)
> +{
> +DWORD tpidrurw = 0;
> +__asm__ volatile ("mrc p15, 0, %0, c13, c0, 2" : "=r" (tpidrurw));
> +return tpidrurw;
> +}
This code is gcc specific and for instance won't compile with PSDK compiler.
It either needs to be protected or replaced with hex/binary form.
--
Dmitry.
WindowTextA).
--
Dmitry.
h is of course an
> improvement, but not worth the risk during code freeze).
Including the patch in 1.6 is certainly up to you, I don't have an actual
application that depends on this.
--
Dmitry.
klmsub1 );
> +ok(res != ERROR_SUCCESS, "test key found in hklm: %d\n", res);
Testing for res != ERROR_SUCCESS is not very useful, please check for
a particular error code. Also please close successfully opened handles.
--
Dmitry.
; > free, and looks now as an actual COM object.
>
> Not really. An actual COM object would contain a pointer to a vtable for
> the correct interface. IUnknown is not much better than void*.
enumx doesn't need anything else besides IUnknown interface, having right
inteface definition won't change anything.
--
Dmitry.
e and decided to fix it since it's
clearly wrong and may lead to other problems.
--
Dmitry.
clear the pending patch state?
--
Dmitry.
t; > if (0==cb) {
> >
> > Why not unconditionally release the lock before the '0==cb' check?
>
> That would cause LeaveCriticalSection be called twice with a race window
> inbetween.
Of course you'd need to remove a redundant LeaveCriticalSection under
'if (0==cb)' case, and I don't see a race there.
--
Dmitry.
asePending = FALSE;
> Malloc32.pSpy = NULL;
> + /* cb == 0 case will release it some lines below. */
> + if (cb) LeaveCriticalSection(&IMalloc32_SpyCS);
> }
>
> if (0==cb) {
Why not unconditionally release the lock before the '0==cb' check?
--
Dmitry.
Daniel Jeliński wrote:
> I'm quite sure you didn't mean to ignore return value of GetStockObject and
> leave logfont uninitialized here.
GetStockObject(DEFAULT_GUI_FONT) can't fail.
--
Dmitry.
Rafał Mużyło wrote:
> Going on an assumption, that 0 is a magic value (standing for 'point'),
> compare against it. Fixes 6ab04040e52465e77558a067309e8a54bdc0f32c regression,
> so would be nice if this got into wine 1.6.
It would be nice to see some test cases first.
--
Dmitry.
nd at least read its copyright
statement. Then you may try to investigate the reasons of bitmap glyph sets,
and what happens without them. Once the investigation step is done you may
try to add glyphs one by one which cover all the discovered issues.
--
Dmitry.
ces at process exit since the whole
process address space is going to be destroyed anyway. Moreover, recently
Alexandre has cleaned up all the DLL entry points in Wine to not free
memory on process exit either since it may lead to dead locks.
--
Dmitry.
run within a library.
What do you mean by "library" in this context?
--
Dmitry.
Marcus Meissner wrote:
> 1030106 Resource leak
> 1030105 Resource leak
> 1030104 Resource leak
These cases are of the kind 'freeing resources at process exit is not useful'.
--
Dmitry.
DELAYLOAD_DESCRIPTOR;
Same question here about IMAGE_DELAYLOAD_DESCRIPTOR. Besides, the structure
contains namesless structure which is not compatible with some compilers and
needs to be fixed.
--
Dmitry.
an initial test accepted it's impossible
to add other tests.
--
Dmitry.
the client side leads to interface
queries on the server side from the class factory instead of the object,
without my test I wouldn't figure that out.
> But I'm sure you can find other ways,
> like using a file in line-buffered mode or things like that. Think about
> it some more.
That would help with avoding a mutex, but mutex is a tiny part of the test.
--
Dmitry.
Dmitry Timoshkov wrote:
> Alexandre Julliard wrote:
>
> > It seems to be awfully complicated. In particular I don't think you need
> > a shared mapping plus a pipe plus a mutex just to log tracing
> > output. Redefining standard macros is also not a good idea.
Ken Thomases wrote:
> ---
> dlls/winemac.drv/cocoa_app.h |2 ++
> dlls/winemac.drv/cocoa_app.m |3 ++-
> 2 files changed, 4 insertions(+), 1 deletions(-)
I wonder, do you actually run 'make test' in dlls/user32/tests with
your changes? My guess is that you don't.
--
Dmitry.
x client and server output is
messed up under Windows. Redefining trace allows to write to the pipe in
the server and synchronize server output on the client side. If you have
a suggestion how to simplify all of this please let me know.
--
Dmitry.
Dmitry Timoshkov wrote:
> With minor clean ups and with increased timeout to wait for server termination
> to please some really slow VMs (that allowed to remove broken() statements).
It would be helpful to provide some feedback and explain the 'pending' state
of the patch w
ameters, in that cases there is no need to take any
> > special action to silence a warning IMHO.
> >
> There is also __attribute__((unused_parameter)) but it's gcc specific.
How is that better than 'unused = unused'? And it's even more typing...
--
Dmitry.
at about action = NULL instead?
A checker tool should be instructed to ignore that kind of a warning
instead. There are many legitimate cases when a function doesn't use
all of its parameters, in that cases there is no need to take any
special action to silence a warning IMHO.
--
Dmitry.
s2.usWinDescent %u/%u\n",
> + font_name, ntm->ntmCellHeight, cell_height, ascent, descent);
> +
> +SetLastError(0xdeadbeef);
The SetLastError() call should be left at its previous position.
--
Dmitry.
; > its
> >> > refcount drops to 0.
> >>
> >> Obviously, but in general the handle will be the only reference.
> >
> > Then I don't understand what you mean regarding the code snippet above.
>
> After release_object, the object will have been freed if this was the
> last reference. You can't keep a pointer to it.
If you know how to fix this properly - please go ahead, I won't be able
to investigate this till next week.
--
Dmitry.
an't be accessed after being released.
> >
> > Being released doesn't mean destroyed. An object gets destroyed only if its
> > refcount drops to 0.
>
> Obviously, but in general the handle will be the only reference.
Then I don't understand what you mean regarding the code snippet above.
--
Dmitry.
> struct object *obj = entry->ptr;
> > -entry->ptr = NULL;
> > if (obj) release_object( obj );
>
> You can't do that, the object can't be accessed after being released.
Being released doesn't mean destroyed. An object gets destroyed only if its
refcount drops to 0.
--
Dmitry.
Christian Costa wrote:
> Out of curiosity. Not freeing these memory allocations shouldn't disturb
> Valgrind to report false memory leaks?
Avoiding heap access on process exit should avoid possible dead locks
in case a terminated by ExitProcess thread holds the heap lock.
--
Dmitry.
1 - 100 of 2809 matches
Mail list logo
