Re: [PATCH 10/16] Simplify config partition management

Mon, 28 May 2018 06:46:55 -0700 


On 2018-05-25 14:01, [ext] Andreas J. Reichel wrote:

From: Andreas Reichel <[email protected]>

* volumes[] already contains all needed data, so instead of
   allocating two additional arrays (roots, root_devpaths),
   just create a mapping config_volumes[] and use
   volumes[config_volumes[]]
* remove CONFIG_PARTITION_MAXCOUNT, since the maximum number of
   config partitions is always the number of volumes present
* simplify error-handling in config loading and writing

Signed-off-by: Andreas Reichel <[email protected]>
---
  env/fatvars.c     | 121 ++++++++++++++++++----------------------------
  env/syspart.c     |  27 +++++------
  include/envdata.h |   2 -
  include/syspart.h |   8 +--
  4 files changed, 60 insertions(+), 98 deletions(-)

diff --git a/env/fatvars.c b/env/fatvars.c
index cefff0c..556b836 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -21,41 +21,27 @@ static BG_ENVDATA env[ENV_NUM_CONFIG_PARTS];

  
  BG_STATUS save_current_config(void)

  {
-       BG_STATUS result = BG_SUCCESS;
+       BG_STATUS result = BG_CONFIG_ERROR;
        EFI_STATUS efistatus;
-       UINTN numHandles = CONFIG_PARTITION_MAXCOUNT;
-       EFI_FILE_HANDLE *roots;
-       EFI_DEVICE_PATH **root_devices;
+       UINTN numHandles = volume_count;
+       UINTN *config_volumes;

  
-	roots = (EFI_FILE_HANDLE *)mmalloc(sizeof(EFI_FILE_HANDLE) *

-                                          CONFIG_PARTITION_MAXCOUNT);
-       if (!roots) {
+       config_volumes = (UINTN *)mmalloc(sizeof(EFI_FILE_HANDLE) *
+                                         volume_count);
+       if (!config_volumes) {
                Print(L"Error, could not allocate memory for config partition "
-                     L"handles.\n");
-               return BG_CONFIG_ERROR;
+                     L"mapping.\n");
+               return result;
        }

  
-	root_devices = (EFI_DEVICE_PATH **)mmalloc(sizeof(EFI_DEVICE_PATH *) *

-                                                  CONFIG_PARTITION_MAXCOUNT);
-       if (!root_devices) {
-               Print(L"Error, could not allocate memory for config partition "
-                     L"device paths.\n");
-               mfree(roots);
-               return BG_CONFIG_ERROR;
-       }
-
-       if (EFI_ERROR(enumerate_cfg_parts(roots, root_devices, &numHandles))) {
+       if (EFI_ERROR(enumerate_cfg_parts(config_volumes, &numHandles))) {
                Print(L"Error, could not enumerate config partitions.\n");
-               mfree(root_devices);
-               mfree(roots);
-               return BG_CONFIG_ERROR;
+               goto scc_cleanup;



Yep that is the kind of thing I don't like in patchsets :) Don't add 
code in one patch and then change it again in one of the next ones.


If you want to fix this, then take a look at 'git add -p'.


You could also split the implementation of the fail-safe from the 
integration and maybe merge that into this patch if that is easier.


Or just merge 9 and 10.


        }

  
-	if (EFI_ERROR(filter_cfg_parts(roots, root_devices, &numHandles))) {

+       if (EFI_ERROR(filter_cfg_parts(config_volumes, &numHandles))) {
                Print(L"Error, could not filter config partitions.\n");
-               mfree(root_devices);
-               mfree(roots);
-               return BG_CONFIG_ERROR;
+               goto scc_cleanup;
        }

  
  	if (numHandles != ENV_NUM_CONFIG_PARTS) {

@@ -64,21 +50,18 @@ BG_STATUS save_current_config(void)
                      numHandles, ENV_NUM_CONFIG_PARTS);
                /* In case of saving, this must be treated as error, to not
                 * overwrite another partition's config file. */
-               mfree(root_devices);
-               mfree(roots);
-               return BG_CONFIG_ERROR;
+               goto scc_cleanup;
        }

  
+	VOLUME_DESC *v = &volumes[config_volumes[current_partition]];

        EFI_FILE_HANDLE fh = NULL;
-       efistatus = open_cfg_file(roots[current_partition], &fh,
-                                 EFI_FILE_MODE_WRITE | EFI_FILE_MODE_READ);
+       efistatus = open_cfg_file(v->root, &fh, EFI_FILE_MODE_WRITE |
+                                 EFI_FILE_MODE_READ);
        if (EFI_ERROR(efistatus)) {
                Print(L"Error, could not open environment file on system "
                      L"partition %d: %r\n",
                      current_partition, efistatus);
-               mfree(root_devices);
-               mfree(roots);
-               return BG_CONFIG_ERROR;
+               goto scc_cleanup;
        }

  
  	UINTN writelen = sizeof(BG_ENVDATA);

@@ -90,63 +73,50 @@ BG_STATUS save_current_config(void)
                                      (VOID *)&env[current_partition]);
        if (EFI_ERROR(efistatus)) {
                Print(L"Error writing environment to file: %r\n", efistatus);
-               result = BG_CONFIG_ERROR;
+               (VOID) close_cfg_file(v->root, fh);
+               goto scc_cleanup;
        }

  
-	if (EFI_ERROR(close_cfg_file(roots[current_partition], fh))) {

+       if (EFI_ERROR(close_cfg_file(v->root, fh))) {
                Print(L"Error, could not close environment config file.\n");
-               result = BG_CONFIG_ERROR;
+               goto scc_cleanup;
        }
-       mfree(root_devices);
-       mfree(roots);
+
+       result = BG_SUCCESS;
+scc_cleanup:
+       mfree(config_volumes);
        return result;
  }

  
  BG_STATUS load_config(BG_LOADER_PARAMS *bglp)

  {
-       BG_STATUS result = BG_SUCCESS;
-       UINTN numHandles = CONFIG_PARTITION_MAXCOUNT;
-       EFI_FILE_HANDLE *roots;
-       EFI_DEVICE_PATH **root_devices;
+       BG_STATUS result = BG_CONFIG_ERROR;
+       UINTN numHandles = volume_count;
+       UINTN *config_volumes;
        UINTN i;
        int env_invalid[ENV_NUM_CONFIG_PARTS] = {0};

  
-	roots = (EFI_FILE_HANDLE *)mmalloc(sizeof(EFI_FILE_HANDLE) *

-                                          CONFIG_PARTITION_MAXCOUNT);
-       if (!roots) {
+       config_volumes = (UINTN *)mmalloc(sizeof(EFI_FILE_HANDLE) *
+                                         volume_count);
+       if (!config_volumes) {
                Print(L"Error, could not allocate memory for config partition "
-                     L"handles.\n");
-               return BG_CONFIG_ERROR;
+                     L"mapping.\n");
+               return result;
        }

  
-	root_devices = (EFI_DEVICE_PATH **)mmalloc(sizeof(EFI_DEVICE_PATH *) *

-                                                  CONFIG_PARTITION_MAXCOUNT);
-       if (!root_devices) {
-               Print(L"Error, could not allocate memory for config partition "
-                     L"device paths.\n");
-               mfree(roots);
-               return BG_CONFIG_ERROR;
-       }
-
-       if (EFI_ERROR(enumerate_cfg_parts(roots, root_devices, &numHandles))) {
+       if (EFI_ERROR(enumerate_cfg_parts(config_volumes, &numHandles))) {
                Print(L"Error, could not enumerate config partitions.\n");
-               mfree(root_devices);
-               mfree(roots);
-               return BG_CONFIG_ERROR;
+               goto lc_cleanup;
        }

  
-	if (EFI_ERROR(filter_cfg_parts(roots, root_devices, &numHandles))) {

+       if (EFI_ERROR(filter_cfg_parts(config_volumes, &numHandles))) {
                Print(L"Error, could not filter config partitions.\n");
-               mfree(root_devices);
-               mfree(roots);
-               return BG_CONFIG_ERROR;
+               goto lc_cleanup;
        }

  
  	if (numHandles > ENV_NUM_CONFIG_PARTS) {

                Print(L"Error, too many config partitions found. Aborting.\n");
-               mfree(root_devices);
-               mfree(roots);
-               return BG_CONFIG_ERROR;
+               goto lc_cleanup;
        }

  
  	if (numHandles < ENV_NUM_CONFIG_PARTS) {

@@ -161,8 +131,9 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
        /* Load all config data */
        for (i = 0; i < numHandles; i++) {
                EFI_FILE_HANDLE fh = NULL;
-               if (EFI_ERROR(open_cfg_file(roots[i], &fh,
-                                           EFI_FILE_MODE_READ))) {
+               VOLUME_DESC *v = &volumes[config_volumes[i]];
+               if (EFI_ERROR(open_cfg_file(v->root, &fh,
+                                           EFI_FILE_MODE_READ))) {
                        Print(L"Warning, could not open environment file on "
                              L"config partition %d\n",
                              i);
@@ -175,7 +146,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
                              L"partition %d.\n",
                              i);
                        env_invalid[i] = 1;
-                       if (EFI_ERROR(close_cfg_file(roots[i], fh))) {
+                       if (EFI_ERROR(close_cfg_file(v->root, fh))) {
                                Print(L"Error, could not close environment "
                                      L"config file.\n");
                        }
@@ -198,7 +169,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
                        result = BG_CONFIG_PARTIALLY_CORRUPTED;
                }

  
-		if (EFI_ERROR(uefi_call_wrapper(fh->Close, 1, fh))) {

+               if (EFI_ERROR(close_cfg_file(v->root, fh))) {
                        Print(L"Error, could not close environment config "
                              L"file.\n");
                        /* Don't abort, so we may still be able to boot a
@@ -263,8 +234,10 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
        Print(L" kernel: %s\n", bglp->payload_path);
        Print(L" args: %s\n", bglp->payload_options);
        Print(L" timeout: %d seconds\n", bglp->timeout);
-       mfree(root_devices);
-       mfree(roots);
+
+       result = BG_SUCCESS;
+lc_cleanup:
+       mfree(config_volumes);
        return result;
  }

  
diff --git a/env/syspart.c b/env/syspart.c

index 86628a0..48bb9a2 100644
--- a/env/syspart.c
+++ b/env/syspart.c
@@ -37,14 +37,12 @@ EFI_STATUS read_cfg_file(EFI_FILE_HANDLE fh, VOID *buffer)
        return uefi_call_wrapper(fh->Read, 3, fh, &readlen, buffer);
  }

  
-EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots,

-                              EFI_DEVICE_PATH **root_devices,
-                              UINTN *numHandles)
+EFI_STATUS enumerate_cfg_parts(UINTN *config_volumes, UINTN *numHandles)
  {
        EFI_STATUS status;
        UINTN rootCount = 0;

  
-	if (!roots || !numHandles) {

+       if (!config_volumes || !numHandles) {
                Print(L"Invalid parameter in system partition enumeration.\n");
                return EFI_INVALID_PARAMETER;
        }
@@ -58,8 +56,7 @@ EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots,
                                       EFI_FILE_MODE_READ);
                if (status == EFI_SUCCESS) {
                        Print(L"Config file found on volume %d.\n", index);
-                       roots[rootCount] = volumes[index].root;
-                       root_devices[rootCount] = volumes[index].devpath;
+                       config_volumes[rootCount] = index;
                        rootCount++;
                        status = close_cfg_file(volumes[index].root, fh);
                        if (EFI_ERROR(status)) {
@@ -74,8 +71,7 @@ EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots,
        return EFI_SUCCESS;
  }

  
-EFI_STATUS filter_cfg_parts(EFI_FILE_HANDLE *roots,

-                           EFI_DEVICE_PATH **root_devices, UINTN *numHandles)
+EFI_STATUS filter_cfg_parts(UINTN *config_volumes, UINTN *numHandles)
  {
        BOOLEAN only_envs_on_bootdevice = FALSE;

  
@@ -84,8 +80,9 @@ EFI_STATUS filter_cfg_parts(EFI_FILE_HANDLE *roots,

                EFI_FILE_HANDLE fh = NULL;
                EFI_STATUS status;
                BG_ENVDATA env;
+               VOLUME_DESC *v = &volumes[config_volumes[index]];

  
-		status = open_cfg_file(roots[index], &fh, EFI_FILE_MODE_READ);

+               status = open_cfg_file(v->root, &fh, EFI_FILE_MODE_READ);
                if (EFI_ERROR(status)) {
                        return status;
                }
@@ -95,12 +92,12 @@ EFI_STATUS filter_cfg_parts(EFI_FILE_HANDLE *roots,
                        return status;
                }

  
-		if (IsOnBootDevice(root_devices[index]) &&

+               if (IsOnBootDevice(v->devpath) &&
                    (env.status_flags & ENV_STATUS_FAILSAFE)) {
                        only_envs_on_bootdevice = TRUE;
                };

  
-		status = close_cfg_file(roots[index], fh);

+               status = close_cfg_file(v->root, fh);
                if (EFI_ERROR(status)) {
                        return status;
                }
@@ -114,13 +111,11 @@ EFI_STATUS filter_cfg_parts(EFI_FILE_HANDLE *roots,
        Print(L"Fail-Safe Mode enabled.\n");
        UINTN index = 0;
        do {
-               if (!IsOnBootDevice(root_devices[index])) {
+               VOLUME_DESC *v = &volumes[config_volumes[index]];
+               if (!IsOnBootDevice(v->devpath)) {
                        for (UINTN j = index; j < *numHandles-1; j++) {
-                               roots[j] = roots[j+1];
-                               root_devices[j] = root_devices[j+1];
+                               config_volumes[j] = config_volumes[j+1];
                        }
-                       mfree(roots[*numHandles-1]);
-                       mfree(root_devices[*numHandles-1]);
                        (*numHandles)--;
                        Print(L"Filtered Config #%d\n", index);
                }
diff --git a/include/envdata.h b/include/envdata.h
index d2fa6bf..46dd718 100644
--- a/include/envdata.h
+++ b/include/envdata.h
@@ -18,8 +18,6 @@
  #define FAT_ENV_FILENAME "BGENV.DAT"
  #define ENV_STRING_LENGTH 255

  
-#define CONFIG_PARTITION_MAXCOUNT 64

-
  #define ENV_STATUS_IN_PROGRESS 1
  #define ENV_STATUS_FAILSAFE 128

  
diff --git a/include/syspart.h b/include/syspart.h

index 269fa25..18b9a23 100644
--- a/include/syspart.h
+++ b/include/syspart.h
@@ -25,11 +25,7 @@ EFI_STATUS open_cfg_file(EFI_FILE_HANDLE root, 
EFI_FILE_HANDLE *fh,
                         UINT64 mode);
  EFI_STATUS close_cfg_file(EFI_FILE_HANDLE root, EFI_FILE_HANDLE fh);
  EFI_STATUS read_cfg_file(EFI_FILE_HANDLE fh, VOID *buffer);
-EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots,
-                              EFI_DEVICE_PATH **root_devices,
-                              UINTN *maxHandles);
-EFI_STATUS filter_cfg_parts(EFI_FILE_HANDLE *roots,
-                           EFI_DEVICE_PATH **root_devices,
-                           UINTN *maxHandles);
+EFI_STATUS enumerate_cfg_parts(UINTN *config_volumes, UINTN *maxHandles);
+EFI_STATUS filter_cfg_parts(UINTN *config_volumes, UINTN *maxHandles);

  
  #endif // __H_SYSPART__




--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: [email protected]

--
You received this message because you are subscribed to the Google Groups "EFI Boot 
Guard" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit 
https://groups.google.com/d/msgid/efibootguard-dev/f25183d8-e860-a7c1-4342-882856df3115%40siemens.com.
For more options, visit https://groups.google.com/d/optout.






















					Reply via email to