[PATCH 1/2] fwts_efi_module: add fwts library helper fuctions for efi_runtime module
Colin Ian King
colin.king at canonical.com
Thu Oct 18 16:18:21 UTC 2012
Thanks Ivan
I've suggested some minor fixes and things to check for.
On 18/10/12 16:52, Ivan Hu wrote:
> These helper fuctions are used for loading, unloading and checking
> the existence of efi_euntime module.
>
> Signed-off-by: Ivan Hu <ivan.hu at canonical.com>
> ---
> src/lib/include/fwts_efi_module.h | 26 ++++++++++
> src/lib/src/Makefile.am | 3 +-
> src/lib/src/fwts_efi_module.c | 99 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 127 insertions(+), 1 deletion(-)
> create mode 100644 src/lib/include/fwts_efi_module.h
> create mode 100644 src/lib/src/fwts_efi_module.c
>
> diff --git a/src/lib/include/fwts_efi_module.h b/src/lib/include/fwts_efi_module.h
> new file mode 100644
> index 0000000..31a3714
> --- /dev/null
> +++ b/src/lib/include/fwts_efi_module.h
> @@ -0,0 +1,26 @@
> +/*
> + * Copyright (C) 2012 Canonical
> + *
> + * 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
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> + *
> + */
> +
> +#ifndef __FWTS_EFI_MODULE_H__
> +#define __FWTS_EFI_MODULE_H__
> +
> +int fwts_lib_efi_runtime_load_module(fwts_framework *fw);
> +int fwts_lib_efi_runtime_unload_module(fwts_framework *fw);
> +
> +#endif
> diff --git a/src/lib/src/Makefile.am b/src/lib/src/Makefile.am
> index ea97cdb..3f8764e 100644
> --- a/src/lib/src/Makefile.am
> +++ b/src/lib/src/Makefile.am
> @@ -60,4 +60,5 @@ libfwts_la_SOURCES = \
> fwts_wakealarm.c \
> fwts_ac_adapter.c \
> fwts_battery.c \
> - fwts_button.c
> + fwts_button.c \
> + fwts_efi_module.c
> diff --git a/src/lib/src/fwts_efi_module.c b/src/lib/src/fwts_efi_module.c
> new file mode 100644
> index 0000000..710bf4d
> --- /dev/null
> +++ b/src/lib/src/fwts_efi_module.c
> @@ -0,0 +1,99 @@
> +/*
> + * Copyright (C) 2012 Canonical
> + *
> + * 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
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> + *
> + */
> +
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +
> +#include "fwts_pipeio.h"
> +
> +static bool module_already_loaded = false;
> +
> +static int check_module_loaded(void)
> +{
> + FILE *fp;
> + char buffer[1024];
I'd prefer an empty line between declarations and the first statement.
> + module_already_loaded = false;
> + if ((fp = fopen("/proc/modules", "r")) != NULL) {
> + while (fgets(buffer, sizeof(buffer), fp) != NULL) {
> + if (strstr(buffer, "efi_runtime") != NULL)
> + module_already_loaded = true;
> + }
how about breaking out once we found the module, saves some cycles :-)
while (fgets(buffer, sizeof(buffer), fp) != NULL) {
if (strstr(buffer, "efi_runtime") != NULL) {
module_already_loaded = true;
break;
}
}
> + fclose(fp);
> + return FWTS_OK;
> + }
> + return FWTS_ERROR;
> +}
> +
> +int fwts_lib_efi_runtime_load_module(fwts_framework *fw)
> +{
> + struct stat statbuf;
> + fwts_list *output;
> +
> + if (check_module_loaded() != FWTS_OK) {
> + fwts_log_error(fw, "Could not open /proc/modules for checking module loaded.");
> + return FWTS_ERROR;
> + }
> +
> + if (!module_already_loaded) {
> + if (fwts_pipe_exec("modprobe efi_runtime", &output) != FWTS_OK) {
> + fwts_log_error(fw, "Load efi_runtime module error.");
> + return FWTS_ERROR;
> + } else {
I see a memory leak. You need:
if (output)
fwts_text_list_free(output);
> + check_module_loaded();
since we discard the return from check_module_loaded() perhaps we should
just give a hint using:
(void)check_module_loaded();
> + if (!module_already_loaded) {
> + fwts_log_error(fw, "Could not load efi_runtime module.");
> + return FWTS_ERROR;
> + }
> + }
> + }
> +
> + if (stat("/dev/efi_runtime", &statbuf)) {
> + fwts_log_error(fw, "Loaded efi_runtime module but /dev/efi_runtime is not present.");
> + return FWTS_ERROR;
> + }
I'm going to be pedantic here. We should also check if this is a char
device too (it is a char device isn't it?), so
if (!S_ISCHR(statbuf.st_mode) {
fwts_log_error(fw, "Loaded efi_runtime module but /dev/efi_runtime is
not a char device.");
return FWTS_ERROR;
}
I'm not sure, but will there be a race between the module being loaded
and the /dev/efi_runtime being created? We may get some false positives
here, perhaps we need to be careful.
> +
> + return FWTS_OK;
> +}
> +
> +
> +int fwts_lib_efi_runtime_unload_module(fwts_framework *fw)
> +{
> + fwts_list *output;
Add a new line here.
> + if (check_module_loaded() != FWTS_OK) {
> + fwts_log_error(fw, "Could not open /proc/modules for checking module loaded.");
> + return FWTS_ERROR;
> + }
> + if (module_already_loaded) {
> + if (fwts_pipe_exec("modprobe -r efi_runtime", &output) != FWTS_OK) {
> + fwts_log_error(fw, "Unload efi_runtime module error.");
> + return FWTS_ERROR;
> + } else {
again, we need to free up output if it's not null.
> + check_module_loaded();
since we discard the return from check_module_loaded() perhaps we should
just give a hint using:
(void)check_module_loaded();
> + if (module_already_loaded) {
> + fwts_log_error(fw, "Could not unload efi_runtime module.");
> + return FWTS_ERROR;
> + }
> + }
> + }
> +
> + return FWTS_OK;
> +}
>
More information about the fwts-devel
mailing list