| Message ID | 1350575529-28509-1-git-send-email-ivan.hu@canonical.com |
|---|---|
| State | Rejected |
| Headers | show |
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; > +} >
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
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; +}
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