[PATCH 2/2] Add support for different power methods to suspend
Colin Ian King
colin.king at canonical.com
Thu Jul 17 09:44:01 UTC 2014
Thanks for the patch Alberto. I've been a bit nit-picky on my first
review (some of it to do with fwts related coding style). I found a
few minor issues, but most of the code looks good to me.
If you can sort out these issues then I will test it on a range of
devices and releases to ensure it works fine too.
Thanks again for this re-working of s3, I appreciate the effort required
to make it work w/o pm-utils.
Colin
On 15/07/14 15:46, Alberto Milone wrote:
> pm-utils is now deprecated and, as a result, we should rely on
> either Logind (where available) or on sysfs.
>
> While autodetection defaults to either Logind or sysfs (preferring
> The former), it is also possible to pass the --pm-method parameter
> to specify one of the following methods:
>
> logind, sysfs, pm-utils
>
> Note: quirks are only available when using pm-utils
>
> This makes a dependency on GLib necessary (only to avoid using
> the C Dbus bindings).
>
> Similar code should be written to handle S4.
>
> The new functions in fwts_pipeio.c and fwts_stringextras.c come
> from Logind's source code.
> ---
> src/acpi/s3/s3.c | 526 +++++++++++++++++++++++++++++++++---
> src/lib/include/fwts_pipeio.h | 6 +
> src/lib/include/fwts_stringextras.h | 1 +
> src/lib/src/fwts_pipeio.c | 108 ++++++++
> src/lib/src/fwts_stringextras.c | 28 ++
> 5 files changed, 630 insertions(+), 39 deletions(-)
>
> diff --git a/src/acpi/s3/s3.c b/src/acpi/s3/s3.c
> index e5b6ef1..6d6d63d 100644
> --- a/src/acpi/s3/s3.c
> +++ b/src/acpi/s3/s3.c
> @@ -28,9 +28,63 @@
> #include <sys/stat.h>
> #include <unistd.h>
> #include <time.h>
> +#include <glib.h>
> +#include <gio/gio.h>
> +
> +enum pm_methods
> +{
> + logind,
> + pm_utils,
> + sysfs,
> + undefined
> +};
> +
> +typedef struct
> +{
> + fwts_framework *fw;
> + time_t t_start;
> + time_t t_end;
> + GDBusProxy *logind_proxy;
> + GDBusConnection *logind_connection;
> + GMainLoop *gmainloop;
> +} fwts_vars;
> +
If possible, I'd prefer functions to be included after all the #define's
and static var declarations, just part of the fwts coding style really.
> +static inline void free_fwts_vars(void *vars)
> +{
> +
> + fwts_vars *var = *(void**)vars;
> +
> + 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;
> + }
> + }
> + free(var);
> + var = NULL;
> +}
> +
> +static inline void freep(void *p)
> +{
> + free(*(void**) p);
> +}
> +
> +#define _cleanup_free_ __attribute__((cleanup(freep)))
> +#define _cleanup_free_fw_ __attribute__((cleanup(free_fwts_vars)))
> +
> +#define PM_SUSPEND_LOGIND "Suspend"
> +#define PM_SUSPEND_HYBRID_LOGIND "HybridSleep"
> +#define PM_SUSPEND_PMUTILS "pm-suspend"
> +#define PM_SUSPEND_HYBRID_PMUTILS "pm-suspend-hybrid"
>
> -#define PM_SUSPEND "pm-suspend"
> -#define PM_SUSPEND_HYBRID "pm-suspend-hybrid"
> #define FWTS_SUSPEND "FWTS_SUSPEND"
> #define FWTS_RESUME "FWTS_RESUME"
>
> @@ -46,6 +100,8 @@ static bool s3_min_max_delay = false;
> static float s3_suspend_time = 15.0; /* Maximum allowed suspend time */
> static float s3_resume_time = 15.0; /* Maximum allowed resume time */
> static bool s3_hybrid = false;
> +static enum pm_methods pm_method = undefined; /* Default pm-method to use to suspend */
> +
>
> static int s3_init(fwts_framework *fw)
> {
> @@ -60,6 +116,361 @@ static int s3_init(fwts_framework *fw)
> return FWTS_OK;
> }
>
> +/* Initialise the Dbus proxy for Logind */
> +static int logind_init_proxy(fwts_vars *fwts_settings)
> +{
> + int status = 0;
> +
> + if (fwts_settings->logind_connection == NULL)
> + fwts_settings->logind_connection = g_bus_get_sync(G_BUS_TYPE_SYSTEM, NULL, NULL);
> +
> + if (fwts_settings->logind_connection == NULL) {
> + status = 1;
> + fwts_log_error(fwts_settings->fw, "Cannot establish a connection to Dbus\n");
> + goto out;
> + }
> +
> + if (fwts_settings->logind_proxy == NULL) {
> + fwts_settings->logind_proxy = g_dbus_proxy_new_sync(fwts_settings->logind_connection,
> + G_DBUS_PROXY_FLAGS_NONE,
> + NULL, "org.freedesktop.login1",
> + "/org/freedesktop/login1",
> + "org.freedesktop.login1.Manager",
> + NULL, NULL);
> + }
> +
> + if (fwts_settings->logind_proxy == NULL) {
> + status = 1;
> + fwts_log_error(fwts_settings->fw, "Cannot establish a connection to login1.Manager\n");
> + goto out;
> + }
> +
> +out:
> + return status;
> +}
> +
> +/* Callback to handle suspend and resume events */
> +static void logind_on_suspend_signal(
> + GDBusConnection *connection,
> + const gchar *sender_name,
> + const gchar *object_path,
> + const gchar *interface_name,
> + const gchar *signal_name,
> + GVariant *parameters,
> + gpointer user_data)
> +{
> + gboolean status;
> + fwts_vars *fwts_settings = (fwts_vars *)user_data;
> +
> + /* Prevent -Werror=unused-parameter from complaining */
> + FWTS_UNUSED(connection);
> + FWTS_UNUSED(sender_name);
> + FWTS_UNUSED(object_path);
> + FWTS_UNUSED(interface_name);
> + FWTS_UNUSED(signal_name);
> +
> + if (!g_variant_is_of_type(parameters, G_VARIANT_TYPE ("(b)"))) {
> + fwts_log_error(fwts_settings->fw, "Suspend type %s\n", g_variant_get_type_string (parameters));
> + return;
> + }
> + else {
> + g_variant_get(parameters, "(b)", &status);
> + fwts_log_info(fwts_settings->fw, "Suspend status: %s\n", status ? "true" : "false");
> +
> + if (status) {
> + time(&(fwts_settings->t_start));
> + (void)fwts_klog_write(fwts_settings->fw, "Starting fwts suspend\n");
> + (void)fwts_klog_write(fwts_settings->fw, FWTS_SUSPEND "\n");
> + }
> + else {
> + time(&(fwts_settings->t_end));
> + (void)fwts_klog_write(fwts_settings->fw, FWTS_RESUME "\n");
> + (void)fwts_klog_write(fwts_settings->fw, "Finished fwts resume\n");
> + /* Let's give the system some time to get back from S3
> + * or Logind will refuse to suspend and shoot both events
> + * without doing anything
> + */
minor quibble, fwts coding style for block comments are:
/*
* Let's give the system ...
* or Logind...
* without ...
*/
> + if (s3_min_delay < 3)
> + sleep(3);
it may be worth logging the fact that the min delay has been skipped
since this is a user defined option that is being overrided here. E.g.
with an fwts_log_info()
> + g_main_loop_quit(fwts_settings->gmainloop);
> + }
> + }
> +}
> +
> +/* Generic function to test supported Logind actions that reply
> + * with a string
> + */
> +static bool logind_can_do_action(fwts_vars *fwts_settings, gchar *action)
> +{
> + GVariant *reply;
> + GError *error = NULL;
> + bool status = false;
> + gchar *response;
> +
> + if (logind_init_proxy(fwts_settings) != 0)
> + return false;
> +
> + reply = g_dbus_proxy_call_sync(fwts_settings->logind_proxy,
> + action,
> + NULL,
> + G_DBUS_CALL_FLAGS_NONE,
> + -1,
> + NULL,
> + &error);
> +
it looks like you are using 4 char tabs where as fwts coding style is
expecting 8 char tabs spacing.
> + if (reply != NULL) {
> + if (!g_variant_is_of_type(reply, G_VARIANT_TYPE ("(s)"))) {
> + fwts_log_error(fwts_settings->fw, "Unexpected response to %s action: %s\n", action,
> + g_variant_get_type_string (reply));
> +
> + g_variant_unref(reply);
> + return status;
> + }
> +
> + g_variant_get(reply, "(&s)", &response);
> + fwts_log_info(fwts_settings->fw, "Response to %s is %s\n", action, response);
> +
> + if (strcmp(response, "challenge") == 0) {
> + fwts_log_error(fwts_settings->fw, "%s action available only after authorisation\n", action);
> + }
> + else if (strcmp(response, "yes") == 0) {
> + fwts_log_info(fwts_settings->fw, "User allowed to execute the %s action\n", action);
> + status = true;
> + }
> + else if (strcmp(response, "no") == 0) {
> + fwts_log_error(fwts_settings->fw, "User not allowed to execute the %s action\n", action);
> + }
> + else if (strcmp(response, "na") == 0) {
> + fwts_log_error(fwts_settings->fw, "Hardware doesn't support %s action\n", action);
> + }
> +
> + g_variant_unref(reply);
> + }
> + else {
> + fwts_log_error(fwts_settings->fw, "Invalid response from Logind on %s action\n", action);
> + g_error_free(error);
> + }
> +
> + return status;
> +}
> +
> +static bool logind_can_suspend(fwts_vars *fwts_settings)
> +{
> + return logind_can_do_action(fwts_settings, "CanSuspend");
> +}
> +
> +static bool logind_can_hybrid_suspend(fwts_vars *fwts_settings)
> +{
> + return logind_can_do_action(fwts_settings, "CanHybridSleep");
> +}
> +
> +static bool sysfs_can_suspend()
I'd prefer: static bool sysfs_can_suspend(void)
> +{
> + return fwts_file_first_line_contains_string("/sys/power/state", "mem");
> +}
> +
> +static bool sysfs_can_hybrid_suspend()
I'd prefer: static bool sysfs_can_hybrid_suspend(void)
> +{
> + bool status;
> +
> + status = fwts_file_first_line_contains_string("/sys/power/state", "disk");
> +
> + if (!status)
> + return 0;
maybe "return false" is better style if this is a bool (not that it
makes much difference in the value being returned).
> +
> + return fwts_file_first_line_contains_string("/sys/power/disk", "suspend");
> +}
> +
> +/* Detect the best available power method */
> +static enum pm_methods detect_pm_method(fwts_vars *fwts_settings)
> +{
> + if (s3_hybrid ? logind_can_hybrid_suspend(fwts_settings) : logind_can_suspend(fwts_settings))
> + return logind;
> + else if (s3_hybrid ? sysfs_can_hybrid_suspend() : sysfs_can_suspend())
> + return sysfs;
> + else
> + return pm_utils;
> +}
> +
> +/* Call Logind to suspend.
> + * action can be either "Suspend" or "HybridSleep"
> + */
> +static gboolean logind_do_suspend(gpointer data)
> +{
> + GError *error = NULL;
> + GVariant *reply;
> + fwts_vars *fwts_settings = (fwts_vars *)data;
> +
> + /* If the loop is not running, return TRUE so as to repeat the operation */
> + if (g_main_loop_is_running (fwts_settings->gmainloop)) {
> +
extraneous empty line
> + gchar *action = s3_hybrid ? PM_SUSPEND_HYBRID_LOGIND : PM_SUSPEND_LOGIND;
> + fwts_log_info(fwts_settings->fw, "Requesting %s action\n", action);
> + reply = g_dbus_proxy_call_sync(fwts_settings->logind_proxy,
> + action,
> + g_variant_new ("(b)",
> + FALSE),
> + G_DBUS_CALL_FLAGS_NONE,
> + -1,
> + NULL,
> + &error);
> +
> + if (reply != NULL) {
> + g_variant_unref(reply);
> + }
> + else {
> + fwts_log_error(fwts_settings->fw, "Error from Logind: %s\n", error->message);
> +
> + g_error_free(error);
> + /* return 1; */
if it's commented out code, can it be removed?
> + }
> +
> + return FALSE;
> +
> + }
> + fwts_log_info(fwts_settings->fw, "Glib loop not ready\n");
> + return TRUE;
> +}
> +
> +/* Start Glib mainloop and listen to suspend/resume events
> + * coming from Logind.
> + * Exit the loop and return the duration after an event.
> + */
> +static int logind_wait_for_suspend_resume(fwts_vars *fwts_settings)
> +{
> + guint subscription_id = 0;
> + int duration = 0;
> +
> + if (logind_init_proxy(fwts_settings) != 0)
> + return 0;
> +
> + subscription_id = g_dbus_connection_signal_subscribe (fwts_settings->logind_connection,
> + "org.freedesktop.login1", /* sender */
> + "org.freedesktop.login1.Manager",
> + "PrepareForSleep",
> + "/org/freedesktop/login1",
> + NULL, /* arg0 */
> + G_DBUS_SIGNAL_FLAGS_NONE,
> + logind_on_suspend_signal,
> + fwts_settings,
> + NULL);
> +
> +
> + fwts_settings->gmainloop = g_main_loop_new(NULL, FALSE);
> + if (fwts_settings->gmainloop) {
> + g_timeout_add(0.1,
> + logind_do_suspend,
> + fwts_settings);
> +
> + g_main_loop_run(fwts_settings->gmainloop);
> + duration = (int)(fwts_settings->t_end - fwts_settings->t_start);
> +
> + /* Optional, as it will be freed together with the struct */
> + g_main_loop_unref(fwts_settings->gmainloop);
> + fwts_settings->gmainloop = NULL;
> + }
> + else {
> + fwts_log_error(fwts_settings->fw, "Failed to start glib mainloop\n");
> + }
> +
> + g_dbus_connection_signal_unsubscribe(fwts_settings->logind_connection, subscription_id);
> +
> + return duration;
> +}
> +
> +/* Write to a file and report the errno in the log */
> +static int write_to_file_with_log(fwts_framework *fw,
> + const gchar *file,
> + const gchar *content)
> +{
> + int status;
> + status = fwts_write_string_file(file, content);
> +
> + if (status != 0)
> + fwts_log_error(fw, "Writing to %s failed with errno %d\n", file, status);
also maybe useful to add strerror(errno) as errno really makes not much
sense to most users.
> +
> + return status;
> +}
> +
> +static int sysfs_do_suspend(fwts_vars *fwts_settings)
> +{
> + int status;
> +
> + if (s3_hybrid) {
> + status = write_to_file_with_log(fwts_settings->fw, "/sys/power/disk", "suspend");
> +
> + if (status != 0)
> + return status;
> +
> + status = write_to_file_with_log(fwts_settings->fw, "/sys/power/state", "disk");
> + }
> + else {
> + status = write_to_file_with_log(fwts_settings->fw, "/sys/power/state", "mem");
> + }
> +
> + return status;
> +}
> +
> +static int wrap_logind_do_suspend(fwts_vars *fwts_settings,
> + int percent,
> + int *duration,
> + char *str)
If possible, can you add "const" where appropriate, e.g. const int
percent, and in other funcs.
> +{
> + FWTS_UNUSED(str);
> + fwts_progress_message(fwts_settings->fw, percent, "(Suspending)");
> +
> + /* This enters a glib mainloop */
> + *duration = logind_wait_for_suspend_resume(fwts_settings);
> + fwts_log_info(fwts_settings->fw, "S3 duration = %d.", *duration);
> + fwts_progress_message(fwts_settings->fw, percent, "(Resumed)");
> +
> + return *duration > 0 ? 0 : 1;
> +}
> +
> +static int wrap_sysfs_do_suspend(fwts_vars *fwts_settings,
> + int percent,
> + int *duration,
> + char *str)
> +{
> + int status;
minor quibble, fwts coding style, newline would be handy here
> + FWTS_UNUSED(str);
> + fwts_progress_message(fwts_settings->fw, percent, "(Suspending)");
> + time(&(fwts_settings->t_start));
> + (void)fwts_klog_write(fwts_settings->fw, "Starting fwts suspend\n");
> + (void)fwts_klog_write(fwts_settings->fw, FWTS_SUSPEND "\n");
> + status = sysfs_do_suspend(fwts_settings);
> + (void)fwts_klog_write(fwts_settings->fw, FWTS_RESUME "\n");
> + (void)fwts_klog_write(fwts_settings->fw, "Finished fwts resume\n");
> + time(&(fwts_settings->t_end));
> + fwts_progress_message(fwts_settings->fw, percent, "(Resumed)");
> +
> + *duration = (int)(fwts_settings->t_end - fwts_settings->t_start);
> +
> + return status;
> +}
> +
> +static int wrap_pmutils_do_suspend(fwts_vars *fwts_settings,
> + int percent,
> + int *duration,
> + char *command)
> +{
> + int status;
minor quibble, fwts coding style, newline would be handy here
> + fwts_progress_message(fwts_settings->fw, percent, "(Suspending)");
> + time(&(fwts_settings->t_start));
> + (void)fwts_klog_write(fwts_settings->fw, "Starting fwts suspend\n");
> + (void)fwts_klog_write(fwts_settings->fw, FWTS_SUSPEND "\n");
> + (void)fwts_exec(command, &status);
> + (void)fwts_klog_write(fwts_settings->fw, FWTS_RESUME "\n");
> + (void)fwts_klog_write(fwts_settings->fw, "Finished fwts resume\n");
> + time(&(fwts_settings->t_end));
> + fwts_progress_message(fwts_settings->fw, percent, "(Resumed)");
> +
> + *duration = (int)(fwts_settings->t_end - fwts_settings->t_start);
> +
> + return status;
> +}
> +
> +
> static int s3_do_suspend_resume(fwts_framework *fw,
> int *hw_errors,
> int *pm_errors,
> @@ -70,54 +481,80 @@ static int s3_do_suspend_resume(fwts_framework *fw,
> int status;
> int duration;
> int differences;
> - time_t t_start;
> - time_t t_end;
> - char *command;
> - char *quirks;
> + _cleanup_free_ char *command = NULL;
> + _cleanup_free_ char *quirks = NULL;
> + _cleanup_free_fw_ fwts_vars * fwts_settings = NULL;
> char buffer[80];
>
> +
> + int (*do_suspend)(fwts_vars *, int, int*, char*);
> +
> + fwts_settings = calloc(1, sizeof(fwts_vars));
needs a check for NULL return and to handle this error accordingly.
Picked up by Coverity Scan.
> + fwts_settings->fw = fw;
> +
> +
remove extraneous empty line above ^
> + if (pm_method == undefined) {
> + /* Autodetection */
> + fwts_log_info(fw, "Detecting the power method.");
> + pm_method = detect_pm_method(fwts_settings);
> + }
> +
> + switch (pm_method) {
> + case logind:
> + fwts_log_info(fw, "Using logind as the default power method.");
> + if (logind_init_proxy(fwts_settings) != 0) {
> + fwts_log_error(fw, "Failure to connect to Logind.");
> + return FWTS_ERROR;
> + }
> + do_suspend = &wrap_logind_do_suspend;
> + break;
> + case pm_utils:
> + fwts_log_info(fw, "Using pm-utils as the default power method.");
> + do_suspend = &wrap_pmutils_do_suspend;
> + break;
> + case sysfs:
> + fwts_log_info(fw, "Using sysfs as the default power method.");
> + do_suspend = &wrap_sysfs_do_suspend;
> + break;
> + default:
> + /* This should never happen */
> + fwts_log_info(fw, "Using sysfs as the default power method.");
> + do_suspend = &wrap_sysfs_do_suspend;
> + break;
> + }
> +
> if (s3_device_check)
> fwts_hwinfo_get(fw, &hwinfo1);
>
> /* Format up pm-suspend command with optional quirking arguments */
> - if (s3_hybrid) {
> - if ((command = fwts_realloc_strcat(NULL, PM_SUSPEND_HYBRID)) == NULL)
> - return FWTS_OUT_OF_MEMORY;
> - } else {
> - if ((command = fwts_realloc_strcat(NULL, PM_SUSPEND)) == NULL)
> - return FWTS_OUT_OF_MEMORY;
> - }
> -
> - if (s3_quirks) {
> - if ((command = fwts_realloc_strcat(command, " ")) == NULL)
> - return FWTS_OUT_OF_MEMORY;
> - if ((quirks = fwts_args_comma_list(s3_quirks)) == NULL) {
> - free(command);
> - return FWTS_OUT_OF_MEMORY;
> + if (pm_method == pm_utils) {
> + if (s3_hybrid) {
> + if ((command = fwts_realloc_strcat(NULL, PM_SUSPEND_HYBRID_PMUTILS)) == NULL)
> + return FWTS_OUT_OF_MEMORY;
> + } else {
> + if ((command = fwts_realloc_strcat(NULL, PM_SUSPEND_PMUTILS)) == NULL)
> + return FWTS_OUT_OF_MEMORY;
> }
> - if ((command = fwts_realloc_strcat(command, quirks)) == NULL) {
> - free(quirks);
> - return FWTS_OUT_OF_MEMORY;
> +
> + /* 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 ((quirks = fwts_args_comma_list(s3_quirks)) == NULL) {
> + return FWTS_OUT_OF_MEMORY;
> + }
> + if ((command = fwts_realloc_strcat(command, quirks)) == NULL) {
> + return FWTS_OUT_OF_MEMORY;
> + }
> }
> - free(quirks);
> }
>
> fwts_wakealarm_trigger(fw, delay);
>
> /* Do S3 here */
> - fwts_progress_message(fw, percent, "(Suspending)");
> - time(&t_start);
> - (void)fwts_klog_write(fw, "Starting fwts suspend\n");
> - (void)fwts_klog_write(fw, FWTS_SUSPEND "\n");
> - (void)fwts_exec(command, &status);
> - (void)fwts_klog_write(fw, FWTS_RESUME "\n");
> - (void)fwts_klog_write(fw, "Finished fwts resume\n");
> - time(&t_end);
> - fwts_progress_message(fw, percent, "(Resumed)");
> - free(command);
> + status = do_suspend(fwts_settings, percent, &duration, command);
>
> - duration = (int)(t_end - t_start);
> - fwts_log_info(fw, "pm-suspend returned %d after %d seconds.", status, duration);
> + fwts_log_info(fw, "pm-action returned %d after %d seconds.", status, duration);
>
> if (s3_device_check) {
> int i;
> @@ -464,9 +901,9 @@ static int s3_options_handler(fwts_framework *fw, int argc, char * const argv[],
> FWTS_UNUSED(argc);
> FWTS_UNUSED(argv);
>
> - switch (option_char) {
> - case 0:
> - switch (long_index) {
> + switch (option_char) {
> + case 0:
> + switch (long_index) {
This indentation makes my head hurt. Can it be fixed up correctly?
> case 0:
> s3_multiple = atoi(optarg);
> break;
> @@ -504,6 +941,16 @@ static int s3_options_handler(fwts_framework *fw, int argc, char * const argv[],
> case 10:
> s3_hybrid = true;
> break;
> + case 11:
> + if (strcmp(optarg, "logind") == 0)
> + pm_method = logind;
> + else if (strcmp(optarg, "pm-utils") == 0)
> + pm_method = pm_utils;
> + else if (strcmp(optarg, "sysfs") == 0)
> + pm_method = sysfs;
> + else
> + return FWTS_ERROR;
> + break;
> }
> }
> return FWTS_OK;
> @@ -521,6 +968,7 @@ static fwts_option s3_options[] = {
> { "s3-suspend-time", "", 1, "Maximum expected suspend time in seconds, e.g. --s3-suspend-time=3.5" },
> { "s3-resume-time", "", 1, "Maximum expected resume time in seconds, e.g. --s3-resume-time=5.1" },
> { "s3-hybrid", "", 0, "Run S3 with hybrid sleep, i.e. saving system states as S4 does." },
> + { "pm-method", "", 1, "Select the power method to use. Accepted values are \"logind\", \"pm-utils\", \"sysfs\""},
> { NULL, NULL, 0, NULL }
> };
>
> diff --git a/src/lib/include/fwts_pipeio.h b/src/lib/include/fwts_pipeio.h
> index 2429e0a..bc9b80c 100644
> --- a/src/lib/include/fwts_pipeio.h
> +++ b/src/lib/include/fwts_pipeio.h
> @@ -20,9 +20,11 @@
> #ifndef __FWTS_PIPEIO_H__
> #define __FWTS_PIPEIO_H__
>
> +#include <stdio.h>
> #include <unistd.h>
> #include <sys/types.h>
> #include <sys/wait.h>
> +#include <stdbool.h>
>
> #include "fwts.h"
>
> @@ -33,5 +35,9 @@ char *fwts_pipe_read(const int fd, ssize_t *length);
> int fwts_pipe_close(const int fd, const pid_t pid);
> int fwts_pipe_exec(const char *command, fwts_list **list, int *status);
> int fwts_exec(const char *command, int *status);
> +int fwts_write_string_to_file(FILE *file, const char *str);
> +int fwts_write_string_file(const char *file_name, const char *str);
> +int fwts_read_file_first_line(const char *file_name, char **line);
> +bool fwts_file_first_line_contains_string(const char *file_name, const char *str);
>
> #endif
> diff --git a/src/lib/include/fwts_stringextras.h b/src/lib/include/fwts_stringextras.h
> index 3d89fe6..0a2b9c7 100644
> --- a/src/lib/include/fwts_stringextras.h
> +++ b/src/lib/include/fwts_stringextras.h
> @@ -24,5 +24,6 @@
>
> void fwts_chop_newline(char *str);
> char *fwts_realloc_strcat(char *orig, const char *newstr);
> +char *fwts_string_endswith(const char *str, const char *postfix);
>
> #endif
> diff --git a/src/lib/src/fwts_pipeio.c b/src/lib/src/fwts_pipeio.c
> index df07295..7c1f719 100644
> --- a/src/lib/src/fwts_pipeio.c
> +++ b/src/lib/src/fwts_pipeio.c
> @@ -1,6 +1,11 @@
> /*
> * Copyright (C) 2010-2014 Canonical
> *
> + * The following functions are derivative work from systemd, and
> + * are covered by Copyright 2010 Lennart Poettering:
> + * fwts_write_string_to_file(), fwts_write_string_file(),
> + * fwts_write_string_file(), fwts_read_file_first_line()
> + *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License
> * as published by the Free Software Foundation; either version 2
> @@ -29,9 +34,26 @@
> #include <sys/types.h>
> #include <sys/wait.h>
> #include <errno.h>
> +#include <limits.h>
> +#include <stdbool.h>
>
> #include "fwts.h"
>
> +static inline void freep(void *p)
> +{
> + free(*(void**) p);
> +}
> +
> +static inline void fclosep(FILE **file)
> +{
> + if (*file)
> + fclose(*file);
> +}
If possible, I'd prefer functions to be included after all the #define's
and static var declarations, just part of the fwts coding style really.
> +
> +
> +#define _cleanup_free_ __attribute__((cleanup(freep)))
> +#define _cleanup_fclose_ __attribute__((cleanup(fclosep)))
> +
> /*
> * fwts_pipe_open()
> * execl a command, return pid in *childpid and
> @@ -184,3 +206,89 @@ int fwts_exec(const char *command, int *status)
> return FWTS_EXEC_ERROR;
> return FWTS_OK;
> }
> +
> +/*
> + * fwts_write_string_to_file()
> + * write a string to a file pointer
> + * Return 0 if writing worked, errno if it failed.
> + */
> +int fwts_write_string_to_file(FILE *file, const char *str)
> +{
> + errno = 0;
> + fputs(str, file);
> + if (!fwts_string_endswith(str, "\n"))
> + fputc('\n', file);
> +
> + fflush(file);
> +
> + if (ferror(file))
> + return errno ? -errno : -EIO;
For fwts lib helper functions I'd prefer FWTS_OK for success or
FWTS_ERROR to indicate errors if possible. The usual fwts lib
functionality is to report the errors at the lowest level with an
fwts_log_error() rather than pass it up and for the caller to report.
This way, we log errors early and they are forced not to be ignored.
e.g, something like:
fwts_log_error(fw, "Failed to write string '%s' to file, error: %d
(%s).", errno, strerror(errno));
> +
> + return 0;
> +}
> +
> +/*
> + * fwts_write_string_file()
> + * write a string to a file
> + * Return 0 if writing worked, errno if it failed.
> + */
> +int fwts_write_string_file(const char *file_name, const char *str) {
> + _cleanup_fclose_ FILE *file = NULL;
> +
> + file = fopen(file_name, "we");
> + if (!file)
> + return -errno;
> +
> + return fwts_write_string_to_file(file, str);
> +}
as above with error handling
> +
> +/*
> + * fwts_read_file_first_line()
> + * read the first line of a file
> + * Return 0 if reading worked, errno if it failed.
> + */
> +int fwts_read_file_first_line(const char *file_name, char **line)
> +{
> + _cleanup_fclose_ FILE *file = NULL;
> + char buffer[LINE_MAX], *temp;
> +
> + file = fopen(file_name, "re");
> + if (!file)
> + return -errno;
> +
> + if (!fgets(buffer, sizeof(buffer), file)) {
> + if (ferror(file))
> + return errno ? -errno : -EIO;
> + buffer[0] = 0;
> + }
> +
> + temp = strdup(buffer);
> + if (!temp)
> + return -ENOMEM;
> +
> + fwts_chop_newline(temp);
> +
> + *line = temp;
> + return 0;
> +}
as above with error handling
> +
> +/*
> + * fwts_file_first_line_contains_string()
> + * read the first line of a file
> + * Return 0 if reading worked, errno if it failed.
> + */
> +bool fwts_file_first_line_contains_string(const char *file_name, const char *str)
> +{
> + _cleanup_free_ char *contents = NULL;
> + int ret;
> +
> + ret = fwts_read_file_first_line(file_name, &contents);
> +
> + if (ret < 0) {
> + fprintf(stderr, "Cannot get the contents of %s. Errno: %d\n",
> + file_name, ret);
as above with error handling, log errors with fwts_log_error rather than
dump to stderr.
> + return 0;
> + }
> +
> + return (strstr(contents, str) != NULL);
> +}
> \ No newline at end of file
> diff --git a/src/lib/src/fwts_stringextras.c b/src/lib/src/fwts_stringextras.c
> index 7c3adac..a46f360 100644
> --- a/src/lib/src/fwts_stringextras.c
> +++ b/src/lib/src/fwts_stringextras.c
> @@ -1,6 +1,10 @@
> /*
> * Copyright (C) 2010-2014 Canonical
> *
> + * The following functions are derivative work from systemd, and
> + * are covered by Copyright 2010 Lennart Poettering:
> + * fwts_string_endswith()
we need to add that info into debian/copyright
> + *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License
> * as published by the Free Software Foundation; either version 2
> @@ -60,3 +64,27 @@ char *fwts_realloc_strcat(char *orig, const char *newstr)
> }
> return orig;
> }
> +
> +/*
> + * fwts_string_endswith()
> + * see if str ends with postfix
> + * return NULL if fails, otherwise return the matched substring
> + */
> +char* fwts_string_endswith(const char *str, const char *postfix)
> +{
> + size_t sl, pl;
> +
> + sl = strlen(str);
> + pl = strlen(postfix);
> +
> + if (pl == 0)
> + return (char*) str + sl;
> +
> + if (sl < pl)
> + return NULL;
> +
> + if (memcmp(str + sl - pl, postfix, pl) != 0)
> + return NULL;
> +
> + return (char*) str + sl - pl;
> +}
> \ No newline at end of file
>
More information about the fwts-devel
mailing list