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...
5 Jul
2016
5 Jul
'16
8:39 p.m.
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.
6 Jul
6 Jul
5:59 p.m.
New subject: [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...
Hi Pierre, thanks for the fast review! I'll respond inline.
On Tue, Jul 5, 2016 at 4:39 PM, Pierre Schweitzer <pierre(a)reactos.org>
wrote:
For shame! I really don't know how that happened, sorry :( It's been fixed.
==============================================================================
> - ReadAttribute(DeviceExt,
DataContext, Read,
(PCHAR)((ULONG_PTR)BitmapData + Read), (ULONG)BitmapDataSize);
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()?
Hi,
Comments inline.
Le 05/07/2016 09:00, tthompson(a)svn.reactos.org a écrit :
==============================================================================
> +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);
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.
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;
> }
+
+ // 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, 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.
_______________________________________________
Ros-dev mailing list
Ros-dev(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev
7:02 p.m.
New subject: [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...
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 <pierre(a)reactos.org>
wrote:
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 <pierre at reactos.org>
System & Network Administrator
Senior Kernel Developer
ReactOS Deutschland e.V.
Hi,
Comments inline.
Le 05/07/2016 09:00, tthompson(a)svn.reactos.org a écrit :
==============================================================================
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()? ---
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?
3094
days inactive
3095
days old
2 comments
2 participants
participants (2)
-
Pierre Schweitzer -
Trevor Thompson