Patchwork [1/2] fwts_efi_module: add fwts library helper fuctions for efi_runtime module

login
register
mail settings
Submitter Ivan Hu
Date Oct. 18, 2012, 3:52 p.m.
Message ID <1350575529-28509-1-git-send-email-ivan.hu@canonical.com>
Download mbox | patch
Permalink /patch/192368/
State Rejected
Headers show

Comments

Ivan Hu - Oct. 18, 2012, 3:52 p.m.
These helper fuctions are used for loading, unloading and checking
the existence of efi_euntime module.

Signed-off-by: Ivan Hu <ivan.hu@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
Colin King - Oct. 18, 2012, 4:18 p.m.
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@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;
> +}
>
Ivan Hu - Oct. 19, 2012, 9:37 a.m.
Hi Colin,
Thanks for your detail review and helpful comments.
I'll modify the code and resend it.


>> +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.
>

yup. thanks for pointing out this. Actually I've tested couple other 
modules with running this code by changing the modules and dev node. The 
results seems no race problem, but I think it is worth keeping it in 
mind. :)

Cheers,
Ivan

Patch

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];
+	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;
+		}
+		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 {
+			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;
+	}
+
+	return FWTS_OK;
+}
+
+
+int fwts_lib_efi_runtime_unload_module(fwts_framework *fw)
+{
+	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 -r efi_runtime", &output) != FWTS_OK) {
+			fwts_log_error(fw, "Unload efi_runtime module error.");
+			return FWTS_ERROR;
+		} else {
+			check_module_loaded();
+			if (module_already_loaded) {
+				fwts_log_error(fw, "Could not unload efi_runtime module.");
+				return FWTS_ERROR;
+			}
+		}
+	}
+
+	return FWTS_OK;
+}