[v1,4/6] qga: removing switch statements, adding run_process_child

This is a cleanup of the resulting code after detaching
pmutils and Linux sys state file logic:

- remove the SUSPEND_MODE_* macros and use an enumeration
instead. At the same time, drop the switch statements
at the start of each function and use the enumeration
index to get the right binary/argument;

- create a new function called run_process_child(). This
function creates a child process and executes a (shell)
command, returning the command return code. This is a common
operation in the pmutils functions and will be used in the
systemd implementation as well, so this function will avoid
code repetition.

There are more places inside commands-posix.c where this new
run_process_child function can also be used, but one step
at a time.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 qga/commands-posix.c | 190 +++++++++++++++++--------------------------
 1 file changed, 76 insertions(+), 114 deletions(-)

Hi

On Tue, Jun 19, 2018 at 9:38 PM, Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
> This is a cleanup of the resulting code after detaching
> pmutils and Linux sys state file logic:
>
> - remove the SUSPEND_MODE_* macros and use an enumeration
> instead. At the same time, drop the switch statements
> at the start of each function and use the enumeration
> index to get the right binary/argument;
>
> - create a new function called run_process_child(). This
> function creates a child process and executes a (shell)
> command, returning the command return code. This is a common

What about using g_spawn_sync() instead?

> operation in the pmutils functions and will be used in the
> systemd implementation as well, so this function will avoid
> code repetition.
>
> There are more places inside commands-posix.c where this new
> run_process_child function can also be used, but one step
> at a time.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  qga/commands-posix.c | 190 +++++++++++++++++--------------------------
>  1 file changed, 76 insertions(+), 114 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index a2870f9ab9..d5e3805ce9 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1438,152 +1438,122 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>  #define LINUX_SYS_STATE_FILE "/sys/power/state"
>  #define SUSPEND_SUPPORTED 0
>  #define SUSPEND_NOT_SUPPORTED 1
> -#define SUSPEND_MODE_DISK 1
> -#define SUSPEND_MODE_RAM 2
> -#define SUSPEND_MODE_HYBRID 3
>
> -static bool pmutils_supports_mode(int suspend_mode, Error **errp)
> +typedef enum {
> +    SUSPEND_MODE_DISK = 0,
> +    SUSPEND_MODE_RAM = 1,
> +    SUSPEND_MODE_HYBRID = 2,
> +} SuspendMode;
> +
> +static int run_process_child(const char *command[], Error **errp)
>  {
>      Error *local_err = NULL;
> -    const char *pmutils_arg;
> -    const char *pmutils_bin = "pm-is-supported";
> -    char *pmutils_path;
> +    char *cmd_path = g_find_program_in_path(command[0]);
>      pid_t pid;
> -    int status;
> -    bool ret = false;
> -
> -    switch (suspend_mode) {
> -
> -    case SUSPEND_MODE_DISK:
> -        pmutils_arg = "--hibernate";
> -        break;
> -    case SUSPEND_MODE_RAM:
> -        pmutils_arg = "--suspend";
> -        break;
> -    case SUSPEND_MODE_HYBRID:
> -        pmutils_arg = "--suspend-hybrid";
> -        break;
> -    default:
> -        return ret;
> -    }
> +    int status, ret = -1;
>
> -    pmutils_path = g_find_program_in_path(pmutils_bin);
> -    if (!pmutils_path) {
> +    if (!cmd_path) {
>          return ret;
>      }
>
>      pid = fork();
>      if (!pid) {
>          setsid();
> -        execle(pmutils_path, pmutils_bin, pmutils_arg, NULL, environ);
>          /*
> -         * If we get here execle() has failed.
> +         * execve receives a char* const argv[] as second arg but we're
> +         * receiving a const char*[]. Since execve does not change the
> +         * array contents it's tolerable to cast here.
>           */
> -        _exit(SUSPEND_NOT_SUPPORTED);
> +        execve(cmd_path, (char* const*)command, environ);
> +        _exit(errno);
>      } else if (pid < 0) {
>          error_setg_errno(errp, errno, "failed to create child process");
> +        ret = EXIT_FAILURE;
>          goto out;
>      }
>
>      ga_wait_child(pid, &status, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> +        ret = EXIT_FAILURE;
>          goto out;
>      }
>
> -    switch (WEXITSTATUS(status)) {
> -    case SUSPEND_SUPPORTED:
> -        ret = true;
> -        goto out;
> -    case SUSPEND_NOT_SUPPORTED:
> -        goto out;
> -    default:
> -        error_setg(errp,
> -                   "the helper program '%s' returned an unexpected exit status"
> -                   " code (%d)", pmutils_path, WEXITSTATUS(status));
> -        goto out;
> -    }
> +    ret = WEXITSTATUS(status);
>
>  out:
> -    g_free(pmutils_path);
> +    g_free(cmd_path);
>      return ret;
>  }
>
> -static void pmutils_suspend(int suspend_mode, Error **errp)
> +static bool pmutils_supports_mode(SuspendMode mode, Error **errp)
>  {
>      Error *local_err = NULL;
> -    const char *pmutils_bin;
> -    char *pmutils_path;
> -    pid_t pid;
> +    const char *pmutils_args[3] = {"--hibernate", "--suspend",
> +                                   "--suspend-hybrid"};
> +    const char *cmd[3] = {"pm-is-supported", pmutils_args[mode], NULL};
>      int status;
>
> -    switch (suspend_mode) {
> -
> -    case SUSPEND_MODE_DISK:
> -        pmutils_bin = "pm-hibernate";
> -        break;
> -    case SUSPEND_MODE_RAM:
> -        pmutils_bin = "pm-suspend";
> -        break;
> -    case SUSPEND_MODE_HYBRID:
> -        pmutils_bin = "pm-suspend-hybrid";
> -        break;
> -    default:
> -        error_setg(errp, "unknown guest suspend mode");
> -        return;
> -    }
> +    status = run_process_child(cmd, &local_err);
>
> -    pmutils_path = g_find_program_in_path(pmutils_bin);
> -    if (!pmutils_path) {
> -        error_setg(errp, "the helper program '%s' was not found", pmutils_bin);
> -        return;
> +    if (status == SUSPEND_SUPPORTED) {
> +        return true;
>      }
>
> -    pid = fork();
> -    if (!pid) {
> -        setsid();
> -        execle(pmutils_path, pmutils_bin, NULL, environ);
> -        /*
> -         * If we get here execle() has failed.
> -         */
> -        _exit(EXIT_FAILURE);
> -    } else if (pid < 0) {
> -        error_setg_errno(errp, errno, "failed to create child process");
> -        goto out;
> +    if (status == -1) {
> +        return false;
>      }
>
> -    ga_wait_child(pid, &status, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> -        goto out;
> +    } else {
> +        error_setg(errp,
> +                   "the helper program '%s' returned an unexpected exit"
> +                   " status code (%d)", "pm-is-supported", status);
>      }
>
> -    if (WEXITSTATUS(status)) {
> -        error_setg(errp,
> -                   "the helper program '%s' returned an unexpected exit status"
> -                   " code (%d)", pmutils_path, WEXITSTATUS(status));
> +    return false;
> +}
> +
> +static void pmutils_suspend(SuspendMode mode, Error **errp)
> +{
> +    Error *local_err = NULL;
> +    const char *pmutils_binaries[3] = {"pm-hibernate", "pm-suspend",
> +                                       "pm-suspend-hybrid"};
> +    const char *cmd[2] = {pmutils_binaries[mode], NULL};
> +    int status;
> +
> +    status = run_process_child(cmd, &local_err);
> +
> +    if (status == 0) {
> +        return;
>      }
>
> -out:
> -    g_free(pmutils_path);
> +    if (status == -1) {
> +        error_setg(errp, "the helper program '%s' was not found",
> +                    pmutils_binaries[mode]);
> +        return;
> +    }
> +
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +    } else {
> +        error_setg(errp,
> +                   "the helper program '%s' returned an unexpected exit"
> +                   " status code (%d)", pmutils_binaries[mode], status);
> +    }
>  }
>
> -static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
> +static bool linux_sys_state_supports_mode(SuspendMode mode, Error **errp)
>  {
> -    const char *sysfile_str;
> +    const char *sysfile_strs[3] = {"disk", "mem", NULL};
> +    const char *sysfile_str = sysfile_strs[mode];
>      char buf[32]; /* hopefully big enough */
>      int fd;
>      ssize_t ret;
>
> -    switch (suspend_mode) {
> -
> -    case SUSPEND_MODE_DISK:
> -        sysfile_str = "disk";
> -        break;
> -    case SUSPEND_MODE_RAM:
> -        sysfile_str = "mem";
> -        break;
> -    default:
> +    if (!sysfile_str) {
> +        error_setg(errp, "unknown guest suspend mode");
>          return false;
>      }
>
> @@ -1604,22 +1574,15 @@ static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
>      return false;
>  }
>
> -static void linux_sys_state_suspend(int suspend_mode, Error **errp)
> +static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
>  {
>      Error *local_err = NULL;
> -    const char *sysfile_str;
> +    const char *sysfile_strs[3] = {"disk", "mem", NULL};
> +    const char *sysfile_str = sysfile_strs[mode];
>      pid_t pid;
>      int status;
>
> -    switch (suspend_mode) {
> -
> -    case SUSPEND_MODE_DISK:
> -        sysfile_str = "disk";
> -        break;
> -    case SUSPEND_MODE_RAM:
> -        sysfile_str = "mem";
> -        break;
> -    default:
> +    if (!sysfile_str) {
>          error_setg(errp, "unknown guest suspend mode");
>          return;
>      }
> @@ -1661,12 +1624,12 @@ static void linux_sys_state_suspend(int suspend_mode, Error **errp)
>
>  }
>
> -static void bios_supports_mode(int suspend_mode, Error **errp)
> +static void bios_supports_mode(SuspendMode mode, Error **errp)
>  {
>      Error *local_err = NULL;
>      bool ret;
>
> -    ret = pmutils_supports_mode(suspend_mode, &local_err);
> +    ret = pmutils_supports_mode(mode, &local_err);
>      if (ret) {
>          return;
>      }
> @@ -1674,32 +1637,31 @@ static void bios_supports_mode(int suspend_mode, Error **errp)
>          error_propagate(errp, local_err);
>          return;
>      }
> -    ret = linux_sys_state_supports_mode(suspend_mode, errp);
> +    ret = linux_sys_state_supports_mode(mode, &local_err);
>      if (!ret) {
>          error_setg(errp,
>                     "the requested suspend mode is not supported by the guest");
> -        return;
>      }
>  }
>
> -static void guest_suspend(int suspend_mode, Error **errp)
> +static void guest_suspend(SuspendMode mode, Error **errp)
>  {
>      Error *local_err = NULL;
>
> -    bios_supports_mode(suspend_mode, &local_err);
> +    bios_supports_mode(mode, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>
> -    pmutils_suspend(suspend_mode, &local_err);
> +    pmutils_suspend(mode, &local_err);
>      if (!local_err) {
>          return;
>      }
>
>      local_err = NULL;
>
> -    linux_sys_state_suspend(suspend_mode, &local_err);
> +    linux_sys_state_suspend(mode, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>      }
> --
> 2.17.1
>
>

On 06/19/2018 08:25 PM, Marc-André Lureau wrote:
> Hi
>
> On Tue, Jun 19, 2018 at 9:38 PM, Daniel Henrique Barboza
> <danielhb413@gmail.com> wrote:
>> This is a cleanup of the resulting code after detaching
>> pmutils and Linux sys state file logic:
>>
>> - remove the SUSPEND_MODE_* macros and use an enumeration
>> instead. At the same time, drop the switch statements
>> at the start of each function and use the enumeration
>> index to get the right binary/argument;
>>
>> - create a new function called run_process_child(). This
>> function creates a child process and executes a (shell)
>> command, returning the command return code. This is a common
> What about using g_spawn_sync() instead?

Nice. I've rewritten run_process_child to work with g_spawn_sync(), sparing
the function from all the code to manage the fork() call and etc.

I'll test it a bit more and then send it in v2.


Thanks,


Daniel

>
>> operation in the pmutils functions and will be used in the
>> systemd implementation as well, so this function will avoid
>> code repetition.
>>
>> There are more places inside commands-posix.c where this new
>> run_process_child function can also be used, but one step
>> at a time.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   qga/commands-posix.c | 190 +++++++++++++++++--------------------------
>>   1 file changed, 76 insertions(+), 114 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index a2870f9ab9..d5e3805ce9 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -1438,152 +1438,122 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>>   #define LINUX_SYS_STATE_FILE "/sys/power/state"
>>   #define SUSPEND_SUPPORTED 0
>>   #define SUSPEND_NOT_SUPPORTED 1
>> -#define SUSPEND_MODE_DISK 1
>> -#define SUSPEND_MODE_RAM 2
>> -#define SUSPEND_MODE_HYBRID 3
>>
>> -static bool pmutils_supports_mode(int suspend_mode, Error **errp)
>> +typedef enum {
>> +    SUSPEND_MODE_DISK = 0,
>> +    SUSPEND_MODE_RAM = 1,
>> +    SUSPEND_MODE_HYBRID = 2,
>> +} SuspendMode;
>> +
>> +static int run_process_child(const char *command[], Error **errp)
>>   {
>>       Error *local_err = NULL;
>> -    const char *pmutils_arg;
>> -    const char *pmutils_bin = "pm-is-supported";
>> -    char *pmutils_path;
>> +    char *cmd_path = g_find_program_in_path(command[0]);
>>       pid_t pid;
>> -    int status;
>> -    bool ret = false;
>> -
>> -    switch (suspend_mode) {
>> -
>> -    case SUSPEND_MODE_DISK:
>> -        pmutils_arg = "--hibernate";
>> -        break;
>> -    case SUSPEND_MODE_RAM:
>> -        pmutils_arg = "--suspend";
>> -        break;
>> -    case SUSPEND_MODE_HYBRID:
>> -        pmutils_arg = "--suspend-hybrid";
>> -        break;
>> -    default:
>> -        return ret;
>> -    }
>> +    int status, ret = -1;
>>
>> -    pmutils_path = g_find_program_in_path(pmutils_bin);
>> -    if (!pmutils_path) {
>> +    if (!cmd_path) {
>>           return ret;
>>       }
>>
>>       pid = fork();
>>       if (!pid) {
>>           setsid();
>> -        execle(pmutils_path, pmutils_bin, pmutils_arg, NULL, environ);
>>           /*
>> -         * If we get here execle() has failed.
>> +         * execve receives a char* const argv[] as second arg but we're
>> +         * receiving a const char*[]. Since execve does not change the
>> +         * array contents it's tolerable to cast here.
>>            */
>> -        _exit(SUSPEND_NOT_SUPPORTED);
>> +        execve(cmd_path, (char* const*)command, environ);
>> +        _exit(errno);
>>       } else if (pid < 0) {
>>           error_setg_errno(errp, errno, "failed to create child process");
>> +        ret = EXIT_FAILURE;
>>           goto out;
>>       }
>>
>>       ga_wait_child(pid, &status, &local_err);
>>       if (local_err) {
>>           error_propagate(errp, local_err);
>> +        ret = EXIT_FAILURE;
>>           goto out;
>>       }
>>
>> -    switch (WEXITSTATUS(status)) {
>> -    case SUSPEND_SUPPORTED:
>> -        ret = true;
>> -        goto out;
>> -    case SUSPEND_NOT_SUPPORTED:
>> -        goto out;
>> -    default:
>> -        error_setg(errp,
>> -                   "the helper program '%s' returned an unexpected exit status"
>> -                   " code (%d)", pmutils_path, WEXITSTATUS(status));
>> -        goto out;
>> -    }
>> +    ret = WEXITSTATUS(status);
>>
>>   out:
>> -    g_free(pmutils_path);
>> +    g_free(cmd_path);
>>       return ret;
>>   }
>>
>> -static void pmutils_suspend(int suspend_mode, Error **errp)
>> +static bool pmutils_supports_mode(SuspendMode mode, Error **errp)
>>   {
>>       Error *local_err = NULL;
>> -    const char *pmutils_bin;
>> -    char *pmutils_path;
>> -    pid_t pid;
>> +    const char *pmutils_args[3] = {"--hibernate", "--suspend",
>> +                                   "--suspend-hybrid"};
>> +    const char *cmd[3] = {"pm-is-supported", pmutils_args[mode], NULL};
>>       int status;
>>
>> -    switch (suspend_mode) {
>> -
>> -    case SUSPEND_MODE_DISK:
>> -        pmutils_bin = "pm-hibernate";
>> -        break;
>> -    case SUSPEND_MODE_RAM:
>> -        pmutils_bin = "pm-suspend";
>> -        break;
>> -    case SUSPEND_MODE_HYBRID:
>> -        pmutils_bin = "pm-suspend-hybrid";
>> -        break;
>> -    default:
>> -        error_setg(errp, "unknown guest suspend mode");
>> -        return;
>> -    }
>> +    status = run_process_child(cmd, &local_err);
>>
>> -    pmutils_path = g_find_program_in_path(pmutils_bin);
>> -    if (!pmutils_path) {
>> -        error_setg(errp, "the helper program '%s' was not found", pmutils_bin);
>> -        return;
>> +    if (status == SUSPEND_SUPPORTED) {
>> +        return true;
>>       }
>>
>> -    pid = fork();
>> -    if (!pid) {
>> -        setsid();
>> -        execle(pmutils_path, pmutils_bin, NULL, environ);
>> -        /*
>> -         * If we get here execle() has failed.
>> -         */
>> -        _exit(EXIT_FAILURE);
>> -    } else if (pid < 0) {
>> -        error_setg_errno(errp, errno, "failed to create child process");
>> -        goto out;
>> +    if (status == -1) {
>> +        return false;
>>       }
>>
>> -    ga_wait_child(pid, &status, &local_err);
>>       if (local_err) {
>>           error_propagate(errp, local_err);
>> -        goto out;
>> +    } else {
>> +        error_setg(errp,
>> +                   "the helper program '%s' returned an unexpected exit"
>> +                   " status code (%d)", "pm-is-supported", status);
>>       }
>>
>> -    if (WEXITSTATUS(status)) {
>> -        error_setg(errp,
>> -                   "the helper program '%s' returned an unexpected exit status"
>> -                   " code (%d)", pmutils_path, WEXITSTATUS(status));
>> +    return false;
>> +}
>> +
>> +static void pmutils_suspend(SuspendMode mode, Error **errp)
>> +{
>> +    Error *local_err = NULL;
>> +    const char *pmutils_binaries[3] = {"pm-hibernate", "pm-suspend",
>> +                                       "pm-suspend-hybrid"};
>> +    const char *cmd[2] = {pmutils_binaries[mode], NULL};
>> +    int status;
>> +
>> +    status = run_process_child(cmd, &local_err);
>> +
>> +    if (status == 0) {
>> +        return;
>>       }
>>
>> -out:
>> -    g_free(pmutils_path);
>> +    if (status == -1) {
>> +        error_setg(errp, "the helper program '%s' was not found",
>> +                    pmutils_binaries[mode]);
>> +        return;
>> +    }
>> +
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +    } else {
>> +        error_setg(errp,
>> +                   "the helper program '%s' returned an unexpected exit"
>> +                   " status code (%d)", pmutils_binaries[mode], status);
>> +    }
>>   }
>>
>> -static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
>> +static bool linux_sys_state_supports_mode(SuspendMode mode, Error **errp)
>>   {
>> -    const char *sysfile_str;
>> +    const char *sysfile_strs[3] = {"disk", "mem", NULL};
>> +    const char *sysfile_str = sysfile_strs[mode];
>>       char buf[32]; /* hopefully big enough */
>>       int fd;
>>       ssize_t ret;
>>
>> -    switch (suspend_mode) {
>> -
>> -    case SUSPEND_MODE_DISK:
>> -        sysfile_str = "disk";
>> -        break;
>> -    case SUSPEND_MODE_RAM:
>> -        sysfile_str = "mem";
>> -        break;
>> -    default:
>> +    if (!sysfile_str) {
>> +        error_setg(errp, "unknown guest suspend mode");
>>           return false;
>>       }
>>
>> @@ -1604,22 +1574,15 @@ static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
>>       return false;
>>   }
>>
>> -static void linux_sys_state_suspend(int suspend_mode, Error **errp)
>> +static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
>>   {
>>       Error *local_err = NULL;
>> -    const char *sysfile_str;
>> +    const char *sysfile_strs[3] = {"disk", "mem", NULL};
>> +    const char *sysfile_str = sysfile_strs[mode];
>>       pid_t pid;
>>       int status;
>>
>> -    switch (suspend_mode) {
>> -
>> -    case SUSPEND_MODE_DISK:
>> -        sysfile_str = "disk";
>> -        break;
>> -    case SUSPEND_MODE_RAM:
>> -        sysfile_str = "mem";
>> -        break;
>> -    default:
>> +    if (!sysfile_str) {
>>           error_setg(errp, "unknown guest suspend mode");
>>           return;
>>       }
>> @@ -1661,12 +1624,12 @@ static void linux_sys_state_suspend(int suspend_mode, Error **errp)
>>
>>   }
>>
>> -static void bios_supports_mode(int suspend_mode, Error **errp)
>> +static void bios_supports_mode(SuspendMode mode, Error **errp)
>>   {
>>       Error *local_err = NULL;
>>       bool ret;
>>
>> -    ret = pmutils_supports_mode(suspend_mode, &local_err);
>> +    ret = pmutils_supports_mode(mode, &local_err);
>>       if (ret) {
>>           return;
>>       }
>> @@ -1674,32 +1637,31 @@ static void bios_supports_mode(int suspend_mode, Error **errp)
>>           error_propagate(errp, local_err);
>>           return;
>>       }
>> -    ret = linux_sys_state_supports_mode(suspend_mode, errp);
>> +    ret = linux_sys_state_supports_mode(mode, &local_err);
>>       if (!ret) {
>>           error_setg(errp,
>>                      "the requested suspend mode is not supported by the guest");
>> -        return;
>>       }
>>   }
>>
>> -static void guest_suspend(int suspend_mode, Error **errp)
>> +static void guest_suspend(SuspendMode mode, Error **errp)
>>   {
>>       Error *local_err = NULL;
>>
>> -    bios_supports_mode(suspend_mode, &local_err);
>> +    bios_supports_mode(mode, &local_err);
>>       if (local_err) {
>>           error_propagate(errp, local_err);
>>           return;
>>       }
>>
>> -    pmutils_suspend(suspend_mode, &local_err);
>> +    pmutils_suspend(mode, &local_err);
>>       if (!local_err) {
>>           return;
>>       }
>>
>>       local_err = NULL;
>>
>> -    linux_sys_state_suspend(suspend_mode, &local_err);
>> +    linux_sys_state_suspend(mode, &local_err);
>>       if (local_err) {
>>           error_propagate(errp, local_err);
>>       }
>> --
>> 2.17.1
>>
>>
>
>