Thomas Faber

  • More
  • CR-66
  • summarized and closed
The change to strlen should also not be necessary. -1 does that

The change to strlen should also not be necessary. -1 does that

Wine won't accept style-only changes anyway so there's no point to doing either

Wine won't accept style-only changes anyway so there's no point to doing either

It looks like WdmAudCleanup will do that for us

It looks like WdmAudCleanup will do that for us

This cast does nothing, it's already the same type

This cast does nothing, it's already the same type

Maybe we should link it to the MixerId or the MixerInfo structure in some way

Maybe we should link it to the MixerId or the MixerInfo structure in some way

This is a strange way to identify an event. ... but I don't have a better suggestion, so I guess this is fine for now.

This is a strange way to identify an event.
... but I don't have a better suggestion, so I guess this is fine for now.

Heh, I didn't notice this was disabled. So disregard the comment above – however a FIXME comment as a reminder to change it back would be good.

Heh, I didn't notice this was disabled. So disregard the comment above – however a FIXME comment as a reminder to change it back would be good.

This doesn't need a cast

This doesn't need a cast

I think this function is broken, but this doesn't look like the right fix. In line 1857 below EventData is in fact used with sizeof(KSEVENTDATA). But then we use it differently afterwards. I think...

I think this function is broken, but this doesn't look like the right fix.
In line 1857 below EventData is in fact used with sizeof(KSEVENTDATA). But then we use it differently afterwards.

I think taking AllocEventData's return value as PKSEVENTDATA KsEventData, using that in the Control() call, and doing PEVENT_NOTIFICATION_DATA EventData = (PEVENT_NOTIFICATION_DATA)(KsEventData + 1) for the other access seems like the correct approach

This doesn't need to be NTAPI... saves 2 lines

This doesn't need to be NTAPI... saves 2 lines

Also a tab https://code.reactos.org/static/olpro3/2static/images/wiki/icons/emoticons/tongue.gif

Also a tab

Nit: there are some tabs here

Nit: there are some tabs here

Implement closing mixer in wdmaud
Implement closing mixer in wdmaud
I can understand that a spinlock might be necessary. I just don't understand why it's for one member, but not the other. (or, here but not in line 133)

I can understand that a spinlock might be necessary. I just don't understand why it's for one member, but not the other. (or, here but not in line 133)

Ah, these are for queue heads and such? Makes sense then.

Ah, these are for queue heads and such? Makes sense then.

But hold the spinlock while reading/modifying WakeIrp, right?

But hold the spinlock while reading/modifying WakeIrp, right?

We have lots of broken code elsewhere, that's not an argument that yours is correct https://code.reactos.org/static/olpro3/2static/images/wiki/icons/emoticons/wink.gif In this particular case, if ...

We have lots of broken code elsewhere, that's not an argument that yours is correct

In this particular case, if this is a second wait wake IRP (i.e. another is already pending), and this IRP is cancelled before line 384 below, we will get here with PdoExtension->WakeIrp != Irp. Alternatively there's a way to get here with PdoExtension->WakeIrp == NULL (in which case this assignment will do nothing).
So really, even an assert would be broken here. You should only set WakeIrp to NULL if it is equal to Irp.

It would mean this duplicate IRP has been cancelled after your Irp->Cancel check above, and the cancel routine was already called (and is waiting to clean up the IRP). So I guess that means just ex...

It would mean this duplicate IRP has been cancelled after your Irp->Cancel check above, and the cancel routine was already called (and is waiting to clean up the IRP). So I guess that means just exiting is correct.

Call this FontEntry or MemEntry or something so it's clear which one is a list entry and which isn't?

Call this FontEntry or MemEntry or something so it's clear which one is a list entry and which isn't?

You want (VOID) here. Empty parentheses in a prototype mean "any number of parameters"

You want (VOID) here. Empty parentheses in a prototype mean "any number of parameters"

Man that's a lot of lists

Man that's a lot of lists

If you move CurrentEntry=CurrentEntry->Flink up to line 201 you can save this workaround and just call RemoveCachedEntry

If you move CurrentEntry=CurrentEntry->Flink up to line 201 you can save this workaround and just call RemoveCachedEntry

Somewhere under sdk/include/reactos/drivers

Somewhere under sdk/include/reactos/drivers

Makes sense. Though it seems strange that it's just in this one if branch

Makes sense. Though it seems strange that it's just in this one if branch

It's okay as-is. It would just save a few unnecessary lines of code.

It's okay as-is. It would just save a few unnecessary lines of code.

If this decrement reaches 0 it might need to set the event, or you could deadlock the cleanup thread

If this decrement reaches 0 it might need to set the event, or you could deadlock the cleanup thread

I just don't see the point in writing another implementation that basically does the same thing, instead of just using the existing functions.

I just don't see the point in writing another implementation that basically does the same thing, instead of just using the existing functions.

magic

magic

magic

magic