ACK: [PATCH] acpi: s3, s3power, s4: replace cleanup free with explicit frees
ivanhu
ivan.hu at canonical.com
Mon Dec 21 09:49:42 UTC 2015
On 2015年12月16日 22:57, Colin King wrote:
> From: Colin Ian King <colin.king at canonical.com>
>
> The fwts idiom is to follow the kernel style of error handling by
> jumping to a common return point that free's resources. Use this
> explicit free'ing error handling idiom rather than using GCC
> cleanup attributes to follow the fwts code style and so that we
> don't get a lot of false positives from various static code
> analysis tools.
>
> Signed-off-by: Colin Ian King <colin.king at canonical.com>
> ---
> src/acpi/s3/s3.c | 41 +++++++++++++++++++++++-----------
> src/acpi/s3power/s3power.c | 16 +++++++++-----
> src/acpi/s4/s4.c | 34 +++++++++++++++++++---------
> src/lib/include/fwts_pm_method.h | 48 +++++++++++-----------------------------
> 4 files changed, 75 insertions(+), 64 deletions(-)
>
> diff --git a/src/acpi/s3/s3.c b/src/acpi/s3/s3.c
> index 8e224cf..3742eec 100644
> --- a/src/acpi/s3/s3.c
> +++ b/src/acpi/s3/s3.c
> @@ -153,9 +153,10 @@ static int s3_do_suspend_resume(fwts_framework *fw,
> int status;
> int duration;
> int differences;
> - _cleanup_free_ char *command = NULL;
> - _cleanup_free_ char *quirks = NULL;
> - _cleanup_free_pm_vars_ fwts_pm_method_vars * fwts_settings = NULL;
> + int rc = FWTS_OK;
> + char *command = NULL;
> + char *quirks = NULL;
> + fwts_pm_method_vars *fwts_settings = NULL;
>
> int (*do_suspend)(fwts_pm_method_vars *, const int, int*, const char*);
>
> @@ -176,7 +177,8 @@ static int s3_do_suspend_resume(fwts_framework *fw,
> fwts_log_info(fw, "Using logind as the default power method.");
> if (fwts_logind_init_proxy(fwts_settings) != 0) {
> fwts_log_error(fw, "Failure to connect to Logind.");
> - return FWTS_ERROR;
> + rc = FWTS_ERROR;
> + goto tidy;
> }
> do_suspend = &wrap_logind_do_suspend;
> break;
> @@ -202,22 +204,30 @@ static int s3_do_suspend_resume(fwts_framework *fw,
> /* Format up pm-suspend command with optional quirking arguments */
> if (fw->pm_method == FWTS_PM_PMUTILS) {
> if (s3_hybrid) {
> - if ((command = fwts_realloc_strcat(NULL, PM_SUSPEND_HYBRID_PMUTILS)) == NULL)
> - return FWTS_OUT_OF_MEMORY;
> + if ((command = fwts_realloc_strcat(NULL, PM_SUSPEND_HYBRID_PMUTILS)) == NULL) {
> + rc = FWTS_OUT_OF_MEMORY;
> + goto tidy;
> + }
> } else {
> - if ((command = fwts_realloc_strcat(NULL, PM_SUSPEND_PMUTILS)) == NULL)
> - return FWTS_OUT_OF_MEMORY;
> + if ((command = fwts_realloc_strcat(NULL, PM_SUSPEND_PMUTILS)) == NULL) {
> + rc = FWTS_OUT_OF_MEMORY;
> + goto tidy;
> + }
> }
>
> /* For now we only support quirks with pm-utils */
> if (s3_quirks) {
> - if ((command = fwts_realloc_strcat(command, " ")) == NULL)
> - return FWTS_OUT_OF_MEMORY;
> + if ((command = fwts_realloc_strcat(command, " ")) == NULL) {
> + rc = FWTS_OUT_OF_MEMORY;
> + goto tidy;
> + }
> if ((quirks = fwts_args_comma_list(s3_quirks)) == NULL) {
> - return FWTS_OUT_OF_MEMORY;
> + rc = FWTS_OUT_OF_MEMORY;
> + goto tidy;
> }
> if ((command = fwts_realloc_strcat(command, quirks)) == NULL) {
> - return FWTS_OUT_OF_MEMORY;
> + rc = FWTS_OUT_OF_MEMORY;
> + goto tidy;
> }
> }
> }
> @@ -294,7 +304,12 @@ static int s3_do_suspend_resume(fwts_framework *fw,
> "enter the requested power saving state.");
> }
>
> - return FWTS_OK;
> +tidy:
> + free(command);
> + free(quirks);
> + free_pm_method_vars(fwts_settings);
> +
> + return rc;
> }
>
> static int s3_scan_times(
> diff --git a/src/acpi/s3power/s3power.c b/src/acpi/s3power/s3power.c
> index ba5f1bb..c9b1c0c 100644
> --- a/src/acpi/s3power/s3power.c
> +++ b/src/acpi/s3power/s3power.c
> @@ -242,6 +242,7 @@ static int s3power_test(fwts_framework *fw)
> {
> int status;
> int duration;
> + int rc = FWTS_OK;
>
> bool offline;
>
> @@ -250,7 +251,7 @@ static int s3power_test(fwts_framework *fw)
> uint32_t capacity_before_mWh;
> uint32_t capacity_after_mWh;
>
> - _cleanup_free_pm_vars_ fwts_pm_method_vars * fwts_settings = NULL;
> + fwts_pm_method_vars *fwts_settings = NULL;
>
> int (*do_suspend)(fwts_pm_method_vars *, const int, int*, const char*);
>
> @@ -278,7 +279,8 @@ static int s3power_test(fwts_framework *fw)
> fwts_log_info(fw, "Using logind as the default power method.");
> if (fwts_logind_init_proxy(fwts_settings) != 0) {
> fwts_log_error(fw, "Failure to connect to Logind.");
> - return FWTS_ERROR;
> + rc = FWTS_ERROR;
> + goto tidy;
> }
> do_suspend = &wrap_logind_do_suspend;
> break;
> @@ -300,11 +302,13 @@ static int s3power_test(fwts_framework *fw)
>
> if (s3power_wait_for_adapter_offline(fw, &offline) == FWTS_ERROR) {
> fwts_log_error(fw, "Cannot check if machine is running on battery, aborting test.");
> - return FWTS_ABORTED;
> + rc = FWTS_ABORTED;
> + goto tidy;
> }
> if (!offline) {
> fwts_log_error(fw, "Machine needs to be running on battery to run test, aborting test.");
> - return FWTS_ABORTED;
> + rc = FWTS_ABORTED;
> + goto tidy;
> }
>
> s3power_get_remaining_capacity(fw, &capacity_before_mAh, &capacity_before_mWh);
> @@ -343,8 +347,10 @@ static int s3power_test(fwts_framework *fw)
> "pm-action encountered an error and also failed to "
> "enter the requested power saving state.");
> }
> +tidy:
> + free_pm_method_vars(fwts_settings);
>
> - return FWTS_OK;
> + return rc;
> }
>
> static int s3power_options_check(fwts_framework *fw)
> diff --git a/src/acpi/s4/s4.c b/src/acpi/s4/s4.c
> index 198fec1..71d5a25 100644
> --- a/src/acpi/s4/s4.c
> +++ b/src/acpi/s4/s4.c
> @@ -178,9 +178,10 @@ static int s4_hibernate(fwts_framework *fw,
> int status;
> int duration;
> int differences;
> - _cleanup_free_ char *command = NULL;
> - _cleanup_free_ char *quirks = NULL;
> - _cleanup_free_pm_vars_ fwts_pm_method_vars * fwts_settings = NULL;
> + int rc = FWTS_OK;
> + char *command = NULL;
> + char *quirks = NULL;
> + fwts_pm_method_vars *fwts_settings = NULL;
>
> int (*do_s4)(fwts_pm_method_vars *, const int, int*, const char*);
>
> @@ -201,7 +202,8 @@ static int s4_hibernate(fwts_framework *fw,
> fwts_log_info(fw, "Using logind as the default power method.");
> if (fwts_logind_init_proxy(fwts_settings) != 0) {
> fwts_log_error(fw, "Failure to connect to Logind.");
> - return FWTS_ERROR;
> + rc = FWTS_ERROR;
> + goto tidy;
> }
> do_s4 = &wrap_logind_do_s4;
> break;
> @@ -226,18 +228,24 @@ static int s4_hibernate(fwts_framework *fw,
>
> if (fw->pm_method == FWTS_PM_PMUTILS) {
> /* Format up pm-hibernate command with optional quirking arguments */
> - if ((command = fwts_realloc_strcat(NULL, PM_HIBERNATE)) == NULL)
> - return FWTS_OUT_OF_MEMORY;
> + if ((command = fwts_realloc_strcat(NULL, PM_HIBERNATE)) == NULL) {
> + rc = FWTS_OUT_OF_MEMORY;
> + goto tidy;
> + }
>
> /* For now we only support quirks with pm-utils */
> if (s4_quirks) {
> - if ((command = fwts_realloc_strcat(command, " ")) == NULL)
> - return FWTS_OUT_OF_MEMORY;
> + if ((command = fwts_realloc_strcat(command, " ")) == NULL) {
> + rc = FWTS_OUT_OF_MEMORY;
> + goto tidy;
> + }
> if ((quirks = fwts_args_comma_list(s4_quirks)) == NULL) {
> - return FWTS_OUT_OF_MEMORY;
> + rc = FWTS_OUT_OF_MEMORY;
> + goto tidy;
> }
> if ((command = fwts_realloc_strcat(command, quirks)) == NULL) {
> - return FWTS_OUT_OF_MEMORY;
> + rc = FWTS_OUT_OF_MEMORY;
> + goto tidy;
> }
> }
> }
> @@ -336,8 +344,12 @@ static int s4_hibernate(fwts_framework *fw,
> fwts_klog_free(klog_pre);
> fwts_klog_free(klog_post);
> fwts_list_free(klog_diff, NULL);
> +tidy:
> + free(command);
> + free(quirks);
> + free_pm_method_vars(fwts_settings);
>
> - return FWTS_OK;
> + return rc;
> }
>
> static int s4_test_multiple(fwts_framework *fw)
> diff --git a/src/lib/include/fwts_pm_method.h b/src/lib/include/fwts_pm_method.h
> index 97041cc..fc8a638 100644
> --- a/src/lib/include/fwts_pm_method.h
> +++ b/src/lib/include/fwts_pm_method.h
> @@ -44,52 +44,30 @@ typedef struct
> int min_delay;
> } fwts_pm_method_vars;
>
> -static inline void free_pm_method_vars(void *);
> -static inline void freep(void *);
> -
> -#define _cleanup_free_pm_vars_ __attribute__((cleanup(free_pm_method_vars)))
> -#define _cleanup_free_ __attribute__((cleanup(freep)))
> -
> #define PM_SUSPEND_LOGIND "Suspend"
> #define PM_SUSPEND_HYBRID_LOGIND "HybridSleep"
> #define PM_HIBERNATE_LOGIND "Hibernate"
>
> #define FWTS_SUSPEND "FWTS_SUSPEND"
> #define FWTS_RESUME "FWTS_RESUME"
> -#define FWTS_HIBERNATE "FWTS_HIBERNATE"
> -#define FWTS_RESUME "FWTS_RESUME"
> +#define FWTS_HIBERNATE "FWTS_HIBERNATE"
> +#define FWTS_RESUME "FWTS_RESUME"
>
> -static inline void free_pm_method_vars(void *vars)
> +static inline void free_pm_method_vars(fwts_pm_method_vars *var)
> {
> - fwts_pm_method_vars *var = *(void**)vars;
> -
> + if (!var)
> + return;
> #if FWTS_ENABLE_LOGIND
> - if (var) {
> - if (var->logind_proxy) {
> - g_object_unref(var->logind_proxy);
> - var->logind_proxy = NULL;
> - }
> - if (var->logind_connection) {
> - g_object_unref(var->logind_connection);
> - var->logind_connection = NULL;
> - }
> - if (var->gmainloop) {
> - g_main_loop_unref(var->gmainloop);
> - var->gmainloop = NULL;
> - }
> - if (var->action) {
> - free(var->action);
> - var->action = NULL;
> - }
> - }
> + if (var->logind_proxy)
> + g_object_unref(var->logind_proxy);
> + if (var->logind_connection)
> + g_object_unref(var->logind_connection);
> + if (var->gmainloop)
> + g_main_loop_unref(var->gmainloop);
> + if (var->action)
> + free(var->action);
> #endif
> free(var);
> - var = NULL;
> -}
> -
> -static inline void freep(void *p)
> -{
> - free(*(void**) p);
> }
>
> int fwts_logind_init_proxy(fwts_pm_method_vars *fwts_settings);
Acked-by: Ivan Hu <ivan.hu at canonical.com>
More information about the fwts-devel
mailing list