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 +InternalSetResidentAtt
Hi, Comments inline. Le 05/07/2016 09:00, tthomp...@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/filesystems/ntfs/mft.c?rev=71820&r1=71819&r2=71820&view=diff > == > --- 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/filesystems/ntfs/volinfo.c?rev=71820&r1=71819&r2=71820&view=diff > == > --- 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 System & Network Administrator Senior Kernel Developer ReactOS Deutschland e.V. smime.p7s Description: Signature cryptographique S/MIME ___ Ros-dev mailing list Ros-dev@reactos.org h
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 +InternalSetResidentAtt
Hi Pierre, thanks for the fast review! I'll respond inline. On Tue, Jul 5, 2016 at 4:39 PM, Pierre Schweitzer wrote: > Hi, > > Comments inline. > > Le 05/07/2016 09:00, tthomp...@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/filesystems/ntfs/mft.c?rev=71820&r1=71819&r2=71820&view=diff > > > == > > --- 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. > That's a good point. The code sort of just evolved that way; I can instead get a pointer to the target record and access its fields directly. > > + > > +// 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. > For shame! I really don't know how that happened, sorry :( It's been fixed. > > > +{ > > +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/filesystems/ntfs/volinfo.c?rev=71820&r1=71819&r2=71820&view=diff > > > == > > --- 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(De
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 +InternalSetResidentAtt
Le 06/07/2016 19:59, Trevor Thompson a écrit : > Hi Pierre, thanks for the fast review! I'll respond inline. > > On Tue, Jul 5, 2016 at 4:39 PM, Pierre Schweitzer > wrote: > >> Hi, >> >> Comments inline. >> >> Le 05/07/2016 09:00, tthomp...@svn.reactos.org a écrit : >> == >>> --- 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? >> > > I'm getting rid of the Read variable, which isn't being used in any > meaningful way. This just removes all references to said variable. It was > just vestigial from this bit of code I copy-pasted from > NtfsGetFreeClusters(): > > /* FIXME: Totally underoptimized! */ > for (; Read < BitmapDataSize; Read += > DeviceExt->NtfsInfo.BytesPerSector) > { > ReadAttribute(DeviceExt, DataContext, Read, > (PCHAR)((ULONG_PTR)BitmapData + Read), DeviceExt->NtfsInfo.BytesPerSector); > } > > By the way, is there any reason I shouldn't heed the comment and replace > this with a single call to ReadAttribute()? I don't recall why I had to do it that way... Perhaps I was hitting a bug somewhere else (perhaps read alignment issues?). But if it doesn't regress anything, just feel free to do it properly. -- Pierre Schweitzer System & Network Administrator Senior Kernel Developer ReactOS Deutschland e.V. smime.p7s Description: Signature cryptographique S/MIME ___ Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev