New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GSoC 2018 - BTRFS boot #743

Merged
merged 4 commits into from Aug 20, 2018

Conversation

Projects
None yet
8 participants
@Extravert-ir
Contributor

Extravert-ir commented Aug 14, 2018

All work that was done during Google Summer of Code 2018.
CORE-13769

@HeisSpiter HeisSpiter self-assigned this Aug 14, 2018

@ROCKNROLLKID

This comment has been minimized.

ROCKNROLLKID commented Aug 14, 2018

Woohoo. No more 4gb file limit anymore.

@HBelusca

This comment has been minimized.

Contributor

HBelusca commented Aug 14, 2018

MSVC fails at:

[00:04:13] c:\reactos-cov\boot\freeldr\freeldr\lib\fs\btrfs.c(467) : error C2065: '__func__' : undeclared identifier
@Doug-Lyons

This comment has been minimized.

Contributor

Doug-Lyons commented Aug 15, 2018

Maybe something like this?

#if defined (_MSC_VER) 
#define __func__ __FUNCTION__
#endif

From: https://stackoverflow.com/questions/2281970/cross-platform-defining-define-for-macros-function-and-func

/* Fail by default */
IoStatus->Status = STATUS_UNSUCCESSFUL;
IoStatus->Information = 0;

This comment has been minimized.

@HeisSpiter

HeisSpiter Aug 15, 2018

Member

I'm really not keen on that change. Could you do the FsRtlCopyWrite2() way?
You first set success, and only set status on error.
See: sdk/lib/drivers/copysup/

@@ -1981,6 +1985,29 @@ InstallExt2BootCodeToDisk(
return Status;
}
/* Obtaining partition info and writing it to bootsector */

This comment has been minimized.

@HeisSpiter

HeisSpiter Aug 15, 2018

Member

Extra line

UCHAR ChunkMapSize;
UCHAR BootDrive;
ULONGLONG PartitionStartLBA;
UCHAR Fill[1521]; // 1536 - 15

This comment has been minimized.

@ThFabba

ThFabba Aug 15, 2018

Contributor

UCHAR Fill[1536 - 15]; is actually valid, so no need for the extra comment ;)
You can also C_ASSERT(sizeof(BTRFS_BOOTSECTOR) == 3 * 512); or some such below the struct definition, to verify that your calculation is correct.

@@ -2552,7 +2579,7 @@ InstallFatBootcodeToPartition(
static
NTSTATUS
InstallExt2BootcodeToPartition(
InstallBtrfsBootcodeToPartition(

This comment has been minimized.

@ThFabba

ThFabba Aug 15, 2018

Contributor

I'm a bit confused by all this ext2 -> btrfs replacement. Is there a reason we can't leave ext2 support untouched and just add btrfs support on top?

This comment has been minimized.

@Extravert-ir

Extravert-ir Aug 17, 2018

Contributor

I have this point. If you don't agree with it, I can change :)

Our current setup implementation doesn't distinguish "Linux" filesystems between each other, and @HBelusca is going to enable new FS detection and installation code some time :)
But now usetup can support only one "Linux" FS and I decided to just change the names because code is almost the same for ext4 or btrfs.
If somebody will implement ext4 support too, he/she can just copy btrfs code and change names once more)

This comment has been minimized.

@yagoulas

yagoulas Aug 17, 2018

Member

Let's duplicate the code, keep the ext2 code and surround it with an #ifdef 0 and add a comment thay for now the only linux partition type recognised by the setup is btrfs.

This comment has been minimized.

@HeisSpiter

HeisSpiter Aug 19, 2018

Member

Taking over the Ext2 is not a big deal. If you remember Peter Hater's patches, this code is broken and he was fixing it in order to get proper setup with ext2. So just dropping it isn't a big issue to me.

jc PrintDiskError // CF set on error (extensions not supported)
cmp bx, HEX(aa55) // BX = AA55h if installed
jne PrintDiskError
test cl, 1 // si = API subset support bitmap

This comment has been minimized.

@ThFabba

ThFabba Aug 15, 2018

Contributor

CX, not SI

This comment has been minimized.

@HBelusca

HBelusca Aug 19, 2018

Contributor

FIXME! :D

@@ -88,6 +88,7 @@ typedef struct _BTRFS_BOOTSECTOR
UCHAR Fill[1521]; // 1536 - 15

This comment has been minimized.

@HBelusca

HBelusca Aug 17, 2018

Contributor

This commit may be merged together with the other one "[USETUP][SETUPLIB] BTRFS boot sector (VBR) now installs correctly. "

@Extravert-ir

This comment has been minimized.

Contributor

Extravert-ir commented Aug 17, 2018

You made me learn how to use git squash and interactive rebase :)

@@ -42,6 +42,7 @@ list(APPEND FREELDR_BOOTLIB_COMMON_SOURCE
lib/fs/fs.c
lib/fs/iso.c
lib/fs/ntfs.c
lib/fs/btrfs.c

This comment has been minimized.

@HBelusca

HBelusca Aug 18, 2018

Contributor

Please place the file in the list in alphabetical order.

@@ -0,0 +1,1288 @@
# structures and utilities for inspecting a binary file with BTRFS filesystem in it
# some code was taken from https://github.com/knorrie/python-btrfs

This comment has been minimized.

@HBelusca

HBelusca Aug 18, 2018

Contributor

You may add your copyright/left here :)

@@ -0,0 +1,35 @@
from btrfs_structures import *

This comment has been minimized.

@HBelusca

HBelusca Aug 18, 2018

Contributor

You may add your copyright/left here :)

* FILE: boot/freeldr/bootsect/btrfs.S
* PURPOSE: BTRFS boot sector
* PROGRAMMERS: Victor Perevertkin
*/

This comment has been minimized.

@HBelusca

HBelusca Aug 18, 2018

Contributor

Since this is a new file, let's take an opportunity to use the new file headers:
https://www.reactos.org/wiki/Coding_Style#File_Structure

This comment has been minimized.

@Extravert-ir

Extravert-ir Aug 18, 2018

Contributor

Such header goes to both headers (.h) and code files (.c) or just to code files?

This comment has been minimized.

@HBelusca

HBelusca Aug 18, 2018

Contributor

Both.

BootDrive:
.byte 0
PartitionStartLBA:
.quad HEX(3f) // hardcoded! usetup has to write proper value here during installation

This comment has been minimized.

@HBelusca

HBelusca Aug 18, 2018

Contributor

So it's the default "63" sectors (you may add this in the comment).

This comment has been minimized.

@Extravert-ir

Extravert-ir Aug 18, 2018

Contributor

Right now it's being always overwritten in installer

.word HEX(aa55) // BootSector signature
SetTreeRoots:
// converting sizes to sectors. We dont need this sizes in bytes

This comment has been minimized.

@HBelusca

HBelusca Aug 18, 2018

Contributor

" don't ", and "size"

This comment has been minimized.

@HBelusca

HBelusca Aug 19, 2018

Contributor

FIXME! :D

This comment has been minimized.

@Extravert-ir

Extravert-ir Aug 19, 2018

Contributor

Damn :)

.ascii "_BHRfS_M"
//.org 1022
// .word HEX(aa55)

This comment has been minimized.

@HBelusca

HBelusca Aug 18, 2018

Contributor

Deprecated stuff?

This comment has been minimized.

@Extravert-ir

Extravert-ir Aug 18, 2018

Contributor

I don't know whether it is needed here or not. We definitely need a signature after the first sector, but what about next ones?

@@ -0,0 +1,1210 @@
#include <freeldr.h>

This comment has been minimized.

@HBelusca

HBelusca Aug 18, 2018

Contributor

Since this is a new file, let's take an opportunity to use the new file headers:
https://www.reactos.org/wiki/Coding_Style#File_Structure
Or at least, you can copy the file header you placed in the btrfs.h file :)

}
const DEVVTBL BtrFsFuncTable =
{

This comment has been minimized.

@HBelusca

HBelusca Aug 18, 2018

Contributor

Please decrease the level of indentation (and use 4-space indent :) )

N = phandle->inode.size;
rd = btrfs_file_read(&BtrFsInfo->FsRoot, phandle->inr, phandle->position, N, Buffer);
if (rd == -1ULL) {

This comment has been minimized.

@HBelusca

HBelusca Aug 18, 2018

Contributor

Please use the non-K&R style : https://www.reactos.org/wiki/Coding_Style#Braces
(This comment applies to all of this file, and possibly to the btrfs.h file as well, unless in the btrfs.h the structs come directly from 3rd-party, in which case they may be left untouched).

This comment has been minimized.

@Extravert-ir

Extravert-ir Aug 18, 2018

Contributor

Have you thought about some automated code style checker? Just asking :)

This comment has been minimized.

@gigaherz

gigaherz Aug 18, 2018

Member

It has been talked about more than once. It would be possible, but there's more than one style in the code depending on the module and where it comes from (win32ss style is different from NT kernel style, and 3rdparty libs have their own, and wine has their own...), so that would mean two things:

  1. we'd have to include style settings per file (in some comment line in the header or such, and
  2. we'd have to make the style inside a file consistent, which is not always the case.

1 is annoying because it introduces a new line into every file, while 2 is annoying because it introduces a lot of whitespace changes to existing code, making it more annoying to follow the file's history.

movzx ebx, word ptr [bp-2]
add eax, ebx // Increment sector to read
adc edx, 0
shl ebx, 5

This comment has been minimized.

@ThFabba

ThFabba Aug 19, 2018

Contributor

This might deserve a comment, it took me a minute to figure out. We shift by 5 here because increasing the segment by 32 will give us an increase in destination pointer by 512 bytes, as desired (i.e. the "other 4 bits" come from the fact that this gets added to the segment)

// Displays a disk error message
// And reboots
PrintDiskError:
mov si,msgDiskError // Bad boot disk message

This comment has been minimized.

@ThFabba

ThFabba Aug 19, 2018

Contributor

The style here usually seems to have a space after the comma

Extravert-ir added some commits Jun 13, 2018

[USETUP][SETUPLIB] Added support for formatting partition in BTRFS an…
…d installing ReactOS on it.

Removed code related to EXT2 boot sector for now.
CORE-13769
[FREELDR][BTRFS] Implemented BTRFS support in Free Loader. Now it sup…
…ports case-insensitive path lookup, symlink folowing and reading uncompressed files.

Volume boot record is also implemented, it supports reading BTRFS tree structures with upto 64k node size.
This support required to change all path in Free Loader to lowercase for better performance.
CORE-13769

@HeisSpiter HeisSpiter merged commit d084793 into reactos:master Aug 20, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Extravert-ir Extravert-ir deleted the Extravert-ir:gsoc2018_all branch Aug 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment