Ros-dev

ros-dev@reactos.org

5789 discussions

Re: [ros-dev] [ros-diffs] [zhu] 71865: Structural, style, correctness fixes.

by Thomas Faber

On 2016-07-08 19:52, zhu(a)svn.reactos.org wrote: > --- branches/GSoC_2016/lwIP-tcpip/drivers/network/tcpip/address.c [iso-8859-1] (original) > +++ branches/GSoC_2016/lwIP-tcpip/drivers/network/tcpip/address.c [iso-8859-1] Fri Jul 8 17:52:42 2016 > @@ -33,6 +33,8 @@ > static LIST_ENTRY AddressListHead; > > /* implementation in testing */ > +/* Must already hold the Context->RequestListLock */ > +/* Context should be in ->FileObject->FsContext */ You know there is a way to actually tell that to MSVC's static analyzer, right? ;) You want something like _IRQL_requires_(DISPATCH_LEVEL) _IRQL_requires_same_ _Requires_lock_held_(((PTCP_CONTEXT)IrpSp->FileObject->FsContext)->RequestListLock) The last one might be a little too complicated to actually include, but the IRQL ones are easy to help indicate things like this. > NTSTATUS > PrepareIrpForCancel( > PIRP Irp,

8 years, 12 months

Re: [ros-dev] [ros-diffs] [gedmurphy] 71862: [NTOS] - Implement IoCreateFileSpecifyDeviceObjectHint - will be needed for fltmgr - Rename IoCreateFile to IopCreateFile and forward both IoCreateFile and IoCreateFileSpecifyDev...

by Alex Ionescu

On Fri, Jul 8, 2016 at 6:18 AM, <gedmurphy(a)svn.reactos.org> wrote: > > + if (OpenPacket->InternalFlags & IOP_USE_TOP_LEVEL_DEVICE_HINT) > + { > + // FIXME: Verify our device object is good to use > + ASSERT(DirectOpen == FALSE); > } You probably want to take a look at IopCheckTopDeviceHint Best regards, Alex Ionescu

8 years, 12 months

Re: [ros-dev] [ros-diffs] [hbelusca] 71819: [RTL]: Follow what I guess might be Alex' coding standards.

by Alex Ionescu

I agree with one-line ifs FWIW... As for the previous code, I think if the file is using separate braces, you shouldn't go around and write new code that follows the no-braces style -- which yes, we do accept. Best regards, Alex Ionescu On Thu, Jul 7, 2016 at 5:25 PM, Timo Kreuzer <timo.kreuzer(a)web.de> wrote: > > According to the "official" ReactOS coding standards, the previous code > was perfectly fine. > Code like "If (Value == 0) Value = FOO;" (in one line) does not match our > coding style rules. (And it's also annoying, when stepping through it with > WinDbg.) > 3 line comments also don't match our coding style :-P > > For reference: https://reactos.org/wiki/Coding_Style > > Timo > > > > Am 05.07.2016 um 16:30 schrieb Alex Ionescu: > > Coding Style - ReactOS Wiki > <https://www.google.com/url?sa=t&rct=j&q=&esrc=s&source=web&cd=2&ved=0ahUKEw…> > > Stop calling them "mine". > > Best regards, > Alex Ionescu > > On Mon, Jul 4, 2016 at 5:38 PM, <hbelusca(a)svn.reactos.org> wrote: > >> Author: hbelusca >> Date: Tue Jul 5 00:38:28 2016 >> New Revision: 71819 >> >> URL: http://svn.reactos.org/svn/reactos?rev=71819&view=rev >> Log: >> [RTL]: Follow what I guess might be Alex' coding standards. >> >> Modified: >> trunk/reactos/sdk/lib/rtl/process.c >> >> Modified: trunk/reactos/sdk/lib/rtl/process.c >> URL: >> http://svn.reactos.org/svn/reactos/trunk/reactos/sdk/lib/rtl/process.c?rev=… >> >> ============================================================================== >> --- trunk/reactos/sdk/lib/rtl/process.c [iso-8859-1] (original) >> +++ trunk/reactos/sdk/lib/rtl/process.c [iso-8859-1] Tue Jul 5 00:38:28 >> 2016 >> @@ -75,7 +75,7 @@ >> PVOID BaseAddress = NULL; >> SIZE_T EnviroSize; >> SIZE_T Size; >> - PWCHAR Environment = 0; >> + PWCHAR Environment = NULL; >> DPRINT("RtlpInitEnvironment(ProcessHandle: %p, Peb: %p Params: >> %p)\n", >> ProcessHandle, Peb, ProcessParameters); >> >> @@ -301,7 +301,9 @@ >> DUPLICATE_SAME_ACCESS | >> DUPLICATE_SAME_ATTRIBUTES); >> if (!NT_SUCCESS(Status)) >> + { >> _SEH2_LEAVE; >> + } >> } >> >> if (ProcessParameters->StandardOutput) >> @@ -315,7 +317,9 @@ >> DUPLICATE_SAME_ACCESS | >> DUPLICATE_SAME_ATTRIBUTES); >> if (!NT_SUCCESS(Status)) >> + { >> _SEH2_LEAVE; >> + } >> } >> >> if (ProcessParameters->StandardError) >> @@ -329,7 +333,9 @@ >> DUPLICATE_SAME_ACCESS | >> DUPLICATE_SAME_ATTRIBUTES); >> if (!NT_SUCCESS(Status)) >> + { >> _SEH2_LEAVE; >> + } >> } >> } >> _SEH2_FINALLY >> >> >> > > > _______________________________________________ > Ros-dev mailing listRos-dev@reactos.orghttp://www.reactos.org/mailman/listinfo/ros-dev > > > > _______________________________________________ > Ros-dev mailing list > Ros-dev(a)reactos.org > http://www.reactos.org/mailman/listinfo/ros-dev > >

8 years, 12 months

Re: [ros-dev] [ros-diffs] [hbelusca] 71864: [FLTMGR]: Remove a MSVC warning: FltObjectDereference, whose prototype is publicly defined in fltkernel.h, doesn't return any value.

by Ged Murphy

This change in incorrect, FltObectDereference does return a value. It's the public definition that's does match the function. -----Original Message----- From: Ros-diffs [mailto:ros-diffs-bounces@reactos.org] On Behalf Of hbelusca(a)svn.reactos.org Sent: 08 July 2016 18:45 To: ros-diffs(a)reactos.org Subject: [ros-diffs] [hbelusca] 71864: [FLTMGR]: Remove a MSVC warning: FltObjectDereference, whose prototype is publicly defined in fltkernel.h, doesn't return any value. Author: hbelusca Date: Fri Jul 8 17:44:59 2016 New Revision: 71864 URL: http://svn.reactos.org/svn/reactos?rev=71864&view=rev Log: [FLTMGR]: Remove a MSVC warning: FltObjectDereference, whose prototype is publicly defined in fltkernel.h, doesn't return any value.

8 years, 12 months

Re: [ros-dev] [ros-diffs] [hbelusca] 71819: [RTL]: Follow what I guess might be Alex' coding standards.

by Alex Ionescu

Coding Style - ReactOS Wiki <https://www.google.com/url?sa=t&rct=j&q=&esrc=s&source=web&cd=2&ved=0ahUKEw…> Stop calling them "mine". Best regards, Alex Ionescu On Mon, Jul 4, 2016 at 5:38 PM, <hbelusca(a)svn.reactos.org> wrote: > Author: hbelusca > Date: Tue Jul 5 00:38:28 2016 > New Revision: 71819 > > URL: http://svn.reactos.org/svn/reactos?rev=71819&view=rev > Log: > [RTL]: Follow what I guess might be Alex' coding standards. > > Modified: > trunk/reactos/sdk/lib/rtl/process.c > > Modified: trunk/reactos/sdk/lib/rtl/process.c > URL: > http://svn.reactos.org/svn/reactos/trunk/reactos/sdk/lib/rtl/process.c?rev=… > > ============================================================================== > --- trunk/reactos/sdk/lib/rtl/process.c [iso-8859-1] (original) > +++ trunk/reactos/sdk/lib/rtl/process.c [iso-8859-1] Tue Jul 5 00:38:28 > 2016 > @@ -75,7 +75,7 @@ > PVOID BaseAddress = NULL; > SIZE_T EnviroSize; > SIZE_T Size; > - PWCHAR Environment = 0; > + PWCHAR Environment = NULL; > DPRINT("RtlpInitEnvironment(ProcessHandle: %p, Peb: %p Params: %p)\n", > ProcessHandle, Peb, ProcessParameters); > > @@ -301,7 +301,9 @@ > DUPLICATE_SAME_ACCESS | > DUPLICATE_SAME_ATTRIBUTES); > if (!NT_SUCCESS(Status)) > + { > _SEH2_LEAVE; > + } > } > > if (ProcessParameters->StandardOutput) > @@ -315,7 +317,9 @@ > DUPLICATE_SAME_ACCESS | > DUPLICATE_SAME_ATTRIBUTES); > if (!NT_SUCCESS(Status)) > + { > _SEH2_LEAVE; > + } > } > > if (ProcessParameters->StandardError) > @@ -329,7 +333,9 @@ > DUPLICATE_SAME_ACCESS | > DUPLICATE_SAME_ATTRIBUTES); > if (!NT_SUCCESS(Status)) > + { > _SEH2_LEAVE; > + } > } > } > _SEH2_FINALLY > > >

8 years, 12 months

Re: [ros-dev] [ros-diffs] [tthompson] 71807: [NTFS][FREELDR] Fix ReadAttribute() and NtfsReadAttribute() in the case when an attribute contains two data runs; Remove extra if statement that prevents second data run from bei...

by Pierre Schweitzer

Actually, this (duplicated) function looks quite poorly designed (not your fault, obviously :-p). The more you fix it, the more it seems natural that the first specific case should be integrated in the while loop for real. That would make the function more robust, with less duplicated code (and thus, less bugs :-p). GSoC note: this is not a "todo" task. This is just an open discussion about a possible improvement if some day someone's bored. Le 04/07/2016 19:02, tthompson(a)svn.reactos.org a écrit : > Author: tthompson > Date: Mon Jul 4 17:02:10 2016 > New Revision: 71807 > > URL: http://svn.reactos.org/svn/reactos?rev=71807&view=rev > Log: > [NTFS][FREELDR] > Fix ReadAttribute() and NtfsReadAttribute() in the case when an attribute contains two data runs; Remove extra if statement that prevents second data run from being read after it's decoded. > > Modified: > branches/GSoC_2016/NTFS/boot/freeldr/freeldr/lib/fs/ntfs.c > branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/mft.c > > Modified: branches/GSoC_2016/NTFS/boot/freeldr/freeldr/lib/fs/ntfs.c > URL: http://svn.reactos.org/svn/reactos/branches/GSoC_2016/NTFS/boot/freeldr/fre… > ============================================================================== > --- branches/GSoC_2016/NTFS/boot/freeldr/freeldr/lib/fs/ntfs.c [iso-8859-1] (original) > +++ branches/GSoC_2016/NTFS/boot/freeldr/freeldr/lib/fs/ntfs.c [iso-8859-1] Mon Jul 4 17:02:10 2016 > @@ -320,9 +320,6 @@ > } > else > DataRunStartLCN = -1; > - > - if (*DataRun == 0) > - return AlreadyRead; > } > > while (Length > 0) > > Modified: branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/mft.c > URL: http://svn.reactos.org/svn/reactos/branches/GSoC_2016/NTFS/drivers/filesyst… > ============================================================================== > --- branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/mft.c [iso-8859-1] (original) > +++ branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/mft.c [iso-8859-1] Mon Jul 4 17:02:10 2016 > @@ -384,9 +384,6 @@ > } > else > DataRunStartLCN = -1; > - > - if (*DataRun == 0) > - return AlreadyRead; > } > > while (Length > 0) > > -- Pierre Schweitzer <pierre at reactos.org> System & Network Administrator Senior Kernel Developer ReactOS Deutschland e.V.

9 years

Re: [ros-dev] [ros-diffs] [tthompson] 71820: [NTFS] Add ability to write to resident attributes. SetAttributeDataLength() - Check if the file is memory mapped before truncating +InternalSetResidentAttributeLength() - Used b...

by Pierre Schweitzer

Hi, Comments inline. Le 05/07/2016 09:00, tthompson(a)svn.reactos.org a écrit : > Modified: branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/mft.c > URL: http://svn.reactos.org/svn/reactos/branches/GSoC_2016/NTFS/drivers/filesyst… > ============================================================================== > --- branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/mft.c [iso-8859-1] (original) > +++ branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/mft.c [iso-8859-1] Tue Jul 5 07:00:43 2016 > @@ -170,6 +172,50 @@ > return AttrRecord->Resident.ValueLength; > } > > +void > +InternalSetResidentAttributeLength(PNTFS_ATTR_CONTEXT AttrContext, > + PFILE_RECORD_HEADER FileRecord, > + ULONG AttrOffset, > + ULONG DataSize) > +{ > + ULONG EndMarker = AttributeEnd; > + ULONG FinalMarker = FILE_RECORD_END; > + ULONG NextAttributeOffset; > + ULONG Offset; > + USHORT Padding; > + > + DPRINT("InternalSetResidentAttributeLength( %p, %p, %lu, %lu )\n", AttrContext, FileRecord, AttrOffset, DataSize); > + > + // update ValueLength Field > + AttrContext->Record.Resident.ValueLength = DataSize; > + Offset = AttrOffset + FIELD_OFFSET(NTFS_ATTR_RECORD, Resident.ValueLength); > + RtlCopyMemory((PCHAR)FileRecord + Offset, &DataSize, sizeof(ULONG)); That part looks a bit over-engineered to me and might be simplified (like not using an intermediate var, nor RtlCopyMemory). Cosmetic, I'd replace PCHAR by ULONG_PTR. Same comment goes to code below doing basically the same thing. > + > + // calculate the record length and end marker offset > + AttrContext->Record.Length = DataSize + AttrContext->Record.Resident.ValueOffset; > + NextAttributeOffset = AttrOffset + AttrContext->Record.Length; > + > + // Ensure NextAttributeOffset is aligned to an 8-byte boundary > + if (NextAttributeOffset % 8 != 0) > + { > + Padding = 8 - (NextAttributeOffset % 8); > + NextAttributeOffset += Padding; > + AttrContext->Record.Length += Padding; > + } > + > + // update the record length > + Offset = AttrOffset + FIELD_OFFSET(NTFS_ATTR_RECORD, Length); > + RtlCopyMemory((PCHAR)FileRecord + Offset, &AttrContext->Record.Length, sizeof(ULONG)); > + > + // write the end marker > + RtlCopyMemory((PCHAR)FileRecord + NextAttributeOffset, &EndMarker, sizeof(ULONG)); > + > + // write the final marker > + Offset = NextAttributeOffset + sizeof(ULONG); > + RtlCopyMemory((PCHAR)FileRecord + Offset, &FinalMarker, sizeof(ULONG)); > + > + FileRecord->BytesInUse = Offset + sizeof(ULONG); > +} > > NTSTATUS > SetAttributeDataLength(PFILE_OBJECT FileObject, > @@ -179,6 +225,18 @@ > PFILE_RECORD_HEADER FileRecord, > PLARGE_INTEGER DataSize) > { > + NTSTATUS Status = STATUS_SUCCESS; > + > + // are we truncating the file? > + if (DataSize->QuadPart < AttributeDataLength(&AttrContext->Record) Unless I'm blind, I'm not sure it builds. > + { > + if (!MmCanFileBeTruncated(FileObject->SectionObjectPointer, (PLARGE_INTEGER)&AllocationSize)) > + { > + DPRINT1("Can't truncate a memory-mapped file!\n"); > + return STATUS_USER_MAPPED_FILE; > + } > + } > + > if (AttrContext->Record.IsNonResident) > { > ULONG BytesPerCluster = Fcb->Vcb->NtfsInfo.BytesPerCluster; > Modified: branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/volinfo.c > URL: http://svn.reactos.org/svn/reactos/branches/GSoC_2016/NTFS/drivers/filesyst… > ============================================================================== > --- branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/volinfo.c [iso-8859-1] (original) > +++ branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/volinfo.c [iso-8859-1] Tue Jul 5 07:00:43 2016 > @@ -158,7 +157,7 @@ > DPRINT1("Total clusters in bitmap: %I64x\n", BitmapDataSize * 8); > DPRINT1("Diff in size: %I64d B\n", ((BitmapDataSize * 8) - DeviceExt->NtfsInfo.ClusterCount) * DeviceExt->NtfsInfo.SectorsPerCluster * DeviceExt->NtfsInfo.BytesPerSector); > > - ReadAttribute(DeviceExt, DataContext, Read, (PCHAR)((ULONG_PTR)BitmapData + Read), (ULONG)BitmapDataSize); > + ReadAttribute(DeviceExt, DataContext, 0, (PCHAR)BitmapData, (ULONG)BitmapDataSize); What's this change? > > RtlInitializeBitMap(&Bitmap, (PULONG)BitmapData, DeviceExt->NtfsInfo.ClusterCount); > FreeClusters = RtlNumberOfClearBits(&Bitmap); > > -- Pierre Schweitzer <pierre at reactos.org> System & Network Administrator Senior Kernel Developer ReactOS Deutschland e.V.

9 years

Re: [ros-dev] [ros-diffs] [hbelusca] 71829: [CLIPBRD]: Use NULL for null pointer. [EVENTVWR]: Add folder icons (needed for later).

by Thomas Faber

Can't event viewer load those icons from shell32 (much like regedit should)? On 2016-07-06 01:42, hbelusca(a)svn.reactos.org wrote: > Author: hbelusca > Date: Tue Jul 5 23:42:40 2016 > New Revision: 71829 > > URL: http://svn.reactos.org/svn/reactos?rev=71829&view=rev > Log: > [CLIPBRD]: Use NULL for null pointer. > [EVENTVWR]: Add folder icons (needed for later). > > Added: > trunk/reactos/base/applications/mscutils/eventvwr/res/folder.ico > - copied unchanged from r71828, trunk/reactos/base/applications/regedit/res/folder.ico > trunk/reactos/base/applications/mscutils/eventvwr/res/folderopen.ico > - copied unchanged from r71828, trunk/reactos/base/applications/regedit/res/folderopen.ico > Modified: > trunk/reactos/base/applications/clipbrd/clipbrd.c > > Modified: trunk/reactos/base/applications/clipbrd/clipbrd.c > URL: http://svn.reactos.org/svn/reactos/trunk/reactos/base/applications/clipbrd/… > ============================================================================== > --- trunk/reactos/base/applications/clipbrd/clipbrd.c [iso-8859-1] (original) > +++ trunk/reactos/base/applications/clipbrd/clipbrd.c [iso-8859-1] Tue Jul 5 23:42:40 2016 > @@ -256,12 +256,12 @@ > > case CMD_ABOUT: > { > + HICON hIcon; > WCHAR szTitle[MAX_STRING_LEN]; > - HICON hIcon; > > hIcon = LoadIconW(Globals.hInstance, MAKEINTRESOURCE(CLIPBRD_ICON)); > LoadStringW(Globals.hInstance, STRING_CLIPBOARD, szTitle, ARRAYSIZE(szTitle)); > - ShellAboutW(Globals.hMainWnd, szTitle, 0, hIcon); > + ShellAboutW(Globals.hMainWnd, szTitle, NULL, hIcon); > DeleteObject(hIcon); > break; > } > >

9 years

Re: [ros-dev] [ros-diffs] [zhu] 71823: Fixed crash on client and server exit: Corrected some NULL data marking and checking. Moved some cleanup code so they actually execute on function failure (incomplete). Removed redunda...

by Thomas Faber

Comments inline. On 2016-07-05 20:35, zhu(a)svn.reactos.org wrote: > --- branches/GSoC_2016/lwIP-tcpip/drivers/network/tcpip/address.c [iso-8859-1] (original) > +++ branches/GSoC_2016/lwIP-tcpip/drivers/network/tcpip/address.c [iso-8859-1] Tue Jul 5 18:35:17 2016 > @@ -357,15 +356,15 @@ > > KeReleaseSpinLockFromDpcLevel(&Context->RequestListLock); > > - if (Context->lwip_tcp_pcb != Context->AddressFile->lwip_tcp_pcb) > + if (Context->lwip_tcp_pcb == Context->AddressFile->lwip_tcp_pcb); You really don't want a semicolon here ;) > { > - tcp_close(Context->lwip_tcp_pcb); > + Context->AddressFile->lwip_tcp_pcb = NULL; > } > + if (!p) > + { > + CopiedLength = 0; > + Status = STATUS_ADDRESS_CLOSED; > + goto BAD; > + } > > if (RemainingDestBytes <= p->len) > { > RtlCopyMemory(CurrentDestLocation, p->payload, RemainingDestBytes); > CopiedLength = RemainingDestBytes; > + Status = STATUS_SUCCESS; > goto RETURN; We normally call labels things like "Exit" and "Failure". > @@ -826,23 +846,24 @@ > > tcp_recved(tpcb, CopiedLength); > I find it confusing not to have Entry = Head->Flink right here. I'm not sure where it is, but unless there's a good reason for it to be somewhere else I'd recommend putting it right in front of the loop. > + while (Entry != Head) > + { > + Request = CONTAINING_RECORD(Entry, TCP_REQUEST, ListEntry); Entry = Entry->Flink? ;) > + if (Request->PendingMode == TCP_REQUEST_PENDING_RECEIVE) > + { > + tcp_recv(tpcb, lwip_tcp_receive_callback); > + break; > + } > + } > @@ -1666,6 +1690,21 @@ > if (!(IsListEmpty(&Context->RequestListHead))) > { > DPRINT1("Disassociating context with outstanding requests\n"); > + Head = &Context->RequestListHead; > + Entry = Head->Flink; > + while (Entry != Head) > + { > + Request = CONTAINING_RECORD(Entry, TCP_REQUEST, ListEntry); Entry = Entry->Flink; > + if (Request->PendingIrp) > + { > + IrpSp = IoGetCurrentIrpStackLocation(Request->PendingIrp); > + DPRINT1("Pending IRP Control Code: %08x\n", IrpSp->MinorFunction); > + } > + else > + { > + DPRINT1("IRP is NULL\n"); > + } > + } > } > KeReleaseSpinLockFromDpcLevel(&Context->RequestListLock); > Thanks. -Thomas

9 years

Re: [ros-dev] [ros-diffs] [hbelusca] 71811: [RTL] - Duplicate the standard (input/output/error) handles from the parent process, when creating a new one. - Properly fail RtlCreateUserProcess if any of the ZwWriteVirtualMemo...

by Alex Ionescu

On Mon, Jul 4, 2016 at 1:28 PM, <hbelusca(a)svn.reactos.org> wrote: > + if (!NT_SUCCESS(Status)) > + _SEH2_LEAVE; > + } > For the love of God can you please follow our coding standards. Also, I'm pretty familiar with this code path -- are you sure that this is the correct place to do this duplication? Best regards, Alex Ionescu

9 years

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

2004

Ros-dev