[haiku-commits] Re: haiku: hrev44620 - in src: preferences/virtualmemory system/kernel/vm

  • From: Axel Dörfler <axeld@xxxxxxxxxxxxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Fri, 07 Sep 2012 09:36:51 +0200

Am 07.09.2012 02:46, schrieb kallisti5@xxxxxxxxxxx:
91390fb: VM Preflet: Add an automatic swap option swap_auto

What is that supposed to achieve? It sounds a bit redundant to me (at least the only advantage I could think of is that it allows to preserve existing manual changes).

f18cace: VM Preflet: More correctly calculate default swap

Ideally, there would be either a syscall for this, or some shared source, so that it's always done in the same way without having to worry.

+       if (file.Read(&fWindowPosition, sizeof(BPoint)) == sizeof(BPoint))
+               return B_OK;
+       else
+               return B_ERROR;
  }

Here and at other places: the 'else' is superfluous and reduces clarity. It's even in the coding style guide since a couple of weeks :-)

+       if (file.SetTo(path.Path(), B_WRITE_ONLY | B_CREATE_FILE | B_ERASE_FILE)
+               != B_OK)
+               return B_ERROR;

Not sure if that's the fault of my mail client: looks like the '!= B_OK' should be indented one tab further.

+
+       file.Write(&fWindowPosition, sizeof(BPoint));
+       return B_OK;
  }


-void
-Settings::_ReadSwapSettings()
+status_t
+Settings::ReadSwapSettings()
  {
        void* settings = load_driver_settings(kVirtualMemorySettings);
-       if (settings != NULL) {
-               SetSwapEnabled(get_driver_boolean_parameter(settings, "vm", 
false, false));
-               const char* swapSize = get_driver_parameter(settings, 
"swap_size", NULL, NULL);
-               SetSwapSize(swapSize ? atoll(swapSize) : 0);
-               
-#ifdef SWAP_VOLUME_IMPLEMENTED
-               // we need to hang onto this one
-               fBadVolName = strdup(get_driver_parameter(settings, 
"swap_volume", NULL, NULL));
-               
-               BVolumeRoster volumeRoster;
-               BVolume temporaryVolume;
-               
-               if (fBadVolName != NULL) {
-                       status_t result = 
volumeRoster.GetNextVolume(&temporaryVolume);
-                       char volumeName[B_FILE_NAME_LENGTH];
-                       while (result != B_BAD_VALUE) {
-                               temporaryVolume.GetName(volumeName);
-                               if (strcmp(volumeName, fBadVolName) == 0
-                                       && temporaryVolume.IsPersistent() && 
volumeName[0]) {
-                                       SetSwapVolume(temporaryVolume);
-                                       break;
-                               }
-                               result = 
volumeRoster.GetNextVolume(&temporaryVolume);
+       if (settings == NULL)
+               return kErrorSettingsNotFound;
+
+       const char* enabled = get_driver_parameter(settings, "vm", NULL, NULL);
+       const char* size = get_driver_parameter(settings, "swap_size", NULL, 
NULL);
+       const char* volume = get_driver_parameter(settings, "swap_volume_name",
+               NULL, NULL);
+       const char* device = get_driver_parameter(settings,
+               "swap_volume_device", NULL, NULL);
+       const char* filesystem = get_driver_parameter(settings,
+               "swap_volume_filesystem", NULL, NULL);
+       const char* capacity = get_driver_parameter(settings,
+               "swap_volume_capacity", NULL, NULL);
+
+       if (enabled == NULL     || size == NULL || device == NULL || volume == 
NULL
+               || capacity == NULL || filesystem == NULL)
+               return kErrorSettingsInvalid;

You leak the settings here. Better use an automatic object deleter.

+       if (bestVol < 0)
+               return kErrorVolumeNotFound;
+       else
+               return B_OK;
+}

Another example.

+       fs_info info;
+       fs_stat_dev(SwapVolume(), &info);

Always a good idea to check for success, or else you might mess up your settings.

Bye,
   Axel.


Other related posts: