[haiku-commits] Re: haiku: hrev53082 - src/system/boot/loader

  • From: Axel Dörfler <axeld@xxxxxxxxxxxxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Tue, 23 Apr 2019 14:42:43 +0200

Am 18/04/2019 um 18:40 schrieb waddlesplash:

ce624c696f56: loader: load all available file/partitioning system modules.
+++ b/src/system/boot/loader/loader.cpp
@@ -5,61 +5,58 @@
#include "loader.h"
-#include "elf.h"
  #include "RootFileSystem.h"
+#include "elf.h"

Didn't know the alphabet was *that* hard :-)

  #ifndef BOOT_ARCH
-#      error BOOT_ARCH has to be defined to differentiate the kernel per 
platform
+#error BOOT_ARCH has to be defined to differentiate the kernel per platform
  #endif

This is how we used to do it everywhere. When did that change?

-static const char *sKernelPaths[][2] = {
-       { KERNEL_PATH, KERNEL_IMAGE },
+static const char* sKernelPaths[][2] = {
+       {KERNEL_PATH, KERNEL_IMAGE},

Could you please always try to separate style changes from actual changes? It makes reading the patch unnecessarily hard.

  static int
  open_maybe_packaged(BootVolume& volume, const char* path, int openMode)
  {
        if (strncmp(path, kSystemDirectoryPrefix, 
strlen(kSystemDirectoryPrefix))
-                       == 0) {
+               == 0) {

We usually do it like it was. Only && and || get a single tab (and I wouldn't mind if we would reconsider that), as they separate terms.

@@ -163,7 +160,8 @@ load_modules_from(BootVolume& volume, const char* path)
status_t status = elf_load_image(modules, name);
                        if (status != B_OK)
-                               dprintf("Could not load \"%s\" error %" B_PRIx32 
"\n", name, status);
+                               dprintf("Could not load \"%s\" error %" B_PRIx32 
"\n", name,
+                                       status);

Missing block for a multi-line statement.

        // and now load all partitioning and file system modules
-       // needed to identify the boot volume
-
-       if (!gBootVolume.GetBool(BOOT_VOLUME_BOOTED_FROM_IMAGE, false)
-               && gBootVolume.GetInt32(BOOT_METHOD, BOOT_METHOD_DEFAULT)
-                       != BOOT_METHOD_CD) {
-               // iterate over the mounted volumes and load their file system
-               Partition *partition;
-               if (gRoot->GetPartitionFor(volume.RootDirectory(), &partition)
-                               == B_OK) {
-                       while (partition != NULL) {
-                               load_module(volume, partition->ModuleName());
-                               partition = partition->Parent();
-                       }
-               }
-       } else {
-               // The boot image should only contain the file system
-               // needed to boot the system, so we just load it.
-               // ToDo: this is separate from the fall back from above
-               //      as this piece will survive a more intelligent module
-               //      loading approach...
-               char path[B_FILE_NAME_LENGTH];
-               snprintf(path, sizeof(path), "%s/%s", sAddonPaths[0], 
"file_systems");
-               load_modules_from(volume, path);
-       }
+       char path[B_FILE_NAME_LENGTH];
+       snprintf(path, sizeof(path), "%s/%s", sAddonPaths[0], "file_systems");
+       load_modules_from(volume, path);
+       snprintf(
+               path, sizeof(path), "%s/%s", sAddonPaths[0], 
"partitioning_systems");

Somewhat weird line break -- maybe move that into a separate function?

+       load_modules_from(volume, path);

I'm not too fond of this sledge hammer approach. If one boot mechanism is not working properly, why do they all have to suffer?

Could you please use Gerrit more? That's exactly what it is made for.

Bye,
   Axel.

Other related posts:

  1. Home
  2. haiku-commits
  3. Archive
  4. 04-2019