Re: [ros-dev] [ros-diffs] [pschweitzer] 60802: [NTOSKRNL] Implement MiSetControlAreaSymbolsLoaded and MiLoadUserSymbols. These are responsible for marking the control area that we've loaded the symbols and for effectively l...

[Timo Kreuzer]

Am 31.10.2013 00:17, schrieb [e-mail]:
> +VOID
> +NTAPI
> +MiSetControlAreaSymbolsLoaded(IN PCONTROL_AREA ControlArea)
> +{
> KIRQL OldIrql;
> ASSERT(KeGetCurrentIrql() <= APC_LEVEL);
> OldIrql = KeAcquireQueuedSpinLock(LockQueuePfnLock);
> ControlArea->u.Flags.DebugSymbolsLoaded |= 1;
> ASSERT(OldIrql <= APC_LEVEL);
> KeReleaseQueuedSpinLock(LockQueuePfnLock, OldIrql);
> ASSERT(KeGetCurrentIrql() <= APC_LEVEL);
>
> +}

No offence, but seriously? The PFN lock to set a flag?

[Pierre Schweitzer]

Hey,

yes, it can sound weird (especially because in some other places where
we use the ControlArea, we don't lock it), BUT, three reasons here:
-> We change flags (let's ensure there's only one change at a time)
-> Windows does it
-> Microsoft documents it:
http://download.microsoft.com/download/7/E/7/7E7662CF-CBEA-470B-A97E-CE7CE0D...

"PFN Lock : The Scope -> All control area, segments, subsections and
prototype PTE usage".

But even themselves criticize such usage (hence its removal in Win7).

Regards,

On 02/11/2013 00:38, Timo Kreuzer wrote:
> Am 31.10.2013 00:17, schrieb [e-mail]:
> > +VOID
> > +NTAPI
> > +MiSetControlAreaSymbolsLoaded(IN PCONTROL_AREA ControlArea)
> > +{
> > KIRQL OldIrql;
> > ASSERT(KeGetCurrentIrql() <= APC_LEVEL);
> > OldIrql = KeAcquireQueuedSpinLock(LockQueuePfnLock);
> > ControlArea->u.Flags.DebugSymbolsLoaded |= 1;
> > ASSERT(OldIrql <= APC_LEVEL);
> > KeReleaseQueuedSpinLock(LockQueuePfnLock, OldIrql);
> > ASSERT(KeGetCurrentIrql() <= APC_LEVEL);
> >
> > +}
>
> No offence, but seriously? The PFN lock to set a flag?
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev
-- 
Pierre Schweitzer<pierre at reactos.org>
System Administrator
ReactOS Foundation

[Timo Kreuzer]

Hi,

While I find using the PFN lock to synchronize access to control areas
ridiculous in general, I wouldn't dare to  expect anyone to design a
better locking architecture.
My point here is basically that for setting a single flag (and derived
from it's name this function has obviously no other purpose than setting
this flag), without any checks or anythig else, using a lock is not how
you do it. And you probably know how much I love the "Windows does it"
argument...

Timo

Am 02.11.2013 10:48, schrieb Pierre Schweitzer:
> Hey,
>
> yes, it can sound weird (especially because in some other places where
> we use the ControlArea, we don't lock it), BUT, three reasons here:
> -> We change flags (let's ensure there's only one change at a time)
> -> Windows does it
> -> Microsoft documents it:
> http://download.microsoft.com/download/7/E/7/7E7662CF-CBEA-470B-A97E-CE7CE0D...
>
> "PFN Lock : The Scope -> All control area, segments, subsections and
> prototype PTE usage".
>
> But even themselves criticize such usage (hence its removal in Win7).
>
> Regards,
>
> On 02/11/2013 00:38, Timo Kreuzer wrote:
> > Am 31.10.2013 00:17, schrieb [e-mail]:
> > > +VOID
> > > +NTAPI
> > > +MiSetControlAreaSymbolsLoaded(IN PCONTROL_AREA ControlArea)
> > > +{
> > > KIRQL OldIrql;
> > > ASSERT(KeGetCurrentIrql() <= APC_LEVEL);
> > > OldIrql = KeAcquireQueuedSpinLock(LockQueuePfnLock);
> > > ControlArea->u.Flags.DebugSymbolsLoaded |= 1;
> > > ASSERT(OldIrql <= APC_LEVEL);
> > > KeReleaseQueuedSpinLock(LockQueuePfnLock, OldIrql);
> > > ASSERT(KeGetCurrentIrql() <= APC_LEVEL);
> > >
> > > +}
> >
> > No offence, but seriously? The PFN lock to set a flag?
> >
> > Ros-dev mailing list
> > [e-mail]
> > http://www.reactos.org/mailman/listinfo/ros-dev
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev