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
