Patchwork [09/26] lib: fwts_battery: remove redundant framework parameter in helper functions

login
register
mail settings
Submitter Colin King
Date Oct. 14, 2012, 8:32 p.m.
Message ID <1350246738-31699-10-git-send-email-colin.king@canonical.com>
Download mbox | patch
Permalink /patch/191399/
State Accepted
Headers show

Comments

Colin King - Oct. 14, 2012, 8:32 p.m.
From: Colin Ian King <colin.king@canonical.com>

We don't require the fwts_framework parameter in the following functions:

fwts_battery_get_count_sys_fs
fwts_battery_get_count_proc_fs
fwts_battery_get_name_sys_fs
fwts_battery_get_name_proc_fs

..so remove it.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/lib/src/fwts_battery.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
Alex Hung - Oct. 15, 2012, 3:34 a.m.
On 10/15/2012 04:32 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> We don't require the fwts_framework parameter in the following functions:
>
> fwts_battery_get_count_sys_fs
> fwts_battery_get_count_proc_fs
> fwts_battery_get_name_sys_fs
> fwts_battery_get_name_proc_fs
>
> ..so remove it.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/lib/src/fwts_battery.c |   16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/src/lib/src/fwts_battery.c b/src/lib/src/fwts_battery.c
> index 656cc33..b792d0b 100644
> --- a/src/lib/src/fwts_battery.c
> +++ b/src/lib/src/fwts_battery.c
> @@ -176,7 +176,7 @@ static int fwts_battery_get_capacity_proc_fs(fwts_framework *fw,
>   	return FWTS_OK;
>   }
>
> -static int fwts_battery_get_count_sys_fs(fwts_framework *fw, DIR *dir, int *count)
> +static int fwts_battery_get_count_sys_fs(DIR *dir, int *count)
>   {
>   	struct dirent *entry;
>   	char path[PATH_MAX];
> @@ -197,7 +197,7 @@ static int fwts_battery_get_count_sys_fs(fwts_framework *fw, DIR *dir, int *coun
>   	return FWTS_OK;
>   }
>
> -static int fwts_battery_get_count_proc_fs(fwts_framework *fw, DIR *dir, int *count)
> +static int fwts_battery_get_count_proc_fs(DIR *dir, int *count)
>   {
>   	struct dirent *entry;
>   	do {
> @@ -208,7 +208,7 @@ static int fwts_battery_get_count_proc_fs(fwts_framework *fw, DIR *dir, int *cou
>   	return FWTS_OK;
>   }
>
> -static int fwts_battery_get_name_sys_fs(fwts_framework *fw, DIR *dir, int index, char *name)
> +static int fwts_battery_get_name_sys_fs(DIR *dir, int index, char *name)
>   {
>   	struct dirent *entry;
>   	char path[PATH_MAX];
> @@ -242,7 +242,7 @@ static int fwts_battery_get_name_sys_fs(fwts_framework *fw, DIR *dir, int index,
>   	return FWTS_ERROR;
>   }
>
> -static int fwts_battery_get_name_proc_fs(fwts_framework *fw, DIR *dir, int index, char *name)
> +static int fwts_battery_get_name_proc_fs(DIR *dir, int index, char *name)
>   {
>   	struct dirent *entry;
>   	int i = 0;
> @@ -591,10 +591,10 @@ int fwts_battery_get_name(fwts_framework *fw, int index, char *name)
>   	DIR *dir;
>
>   	if ((dir = opendir(FWTS_SYS_CLASS_POWER_SUPPLY)) != NULL) {
> -		ret = fwts_battery_get_name_sys_fs(fw, dir, index, name);
> +		ret = fwts_battery_get_name_sys_fs(dir, index, name);
>   		closedir(dir);
>   	} else if ((dir = opendir(FWTS_PROC_ACPI_BATTERY)) != NULL) {
> -		ret = fwts_battery_get_name_proc_fs(fw, dir, index, name);
> +		ret = fwts_battery_get_name_proc_fs(dir, index, name);
>   		closedir(dir);
>   	} else {
>   		return FWTS_ERROR;
> @@ -609,10 +609,10 @@ int fwts_battery_get_count(fwts_framework *fw, int *count)
>   	DIR *dir;
>
>   	if ((dir = opendir(FWTS_SYS_CLASS_POWER_SUPPLY)) != NULL) {
> -		ret = fwts_battery_get_count_sys_fs(fw, dir, count);
> +		ret = fwts_battery_get_count_sys_fs(dir, count);
>   		closedir(dir);
>   	} else if ((dir = opendir(FWTS_PROC_ACPI_BATTERY)) != NULL) {
> -		ret = fwts_battery_get_count_proc_fs(fw, dir, count);
> +		ret = fwts_battery_get_count_proc_fs(dir, count);
>   		closedir(dir);
>   	} else {
>   		return FWTS_ERROR;
>

Functions such as fwts_battery_get_capacity, 
fwts_battery_get_capacity_proc_fs and fwts_battery_get_capacity_sys_fs 
still have the fwts_framework parameter and it enables *fwts_log_info* 
to log.

Even though fwts_framework is not used now, will it still be better to 
keep them for consistency? (Removing them and adding them back when 
necessary are just as good. It is just to a second opinion.)

Cheers,
Alex Hung
Colin King - Oct. 15, 2012, 8:10 a.m.
On 15/10/12 04:34, Alex Hung wrote:
> On 10/15/2012 04:32 AM, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> We don't require the fwts_framework parameter in the following functions:
>>
>> fwts_battery_get_count_sys_fs
>> fwts_battery_get_count_proc_fs
>> fwts_battery_get_name_sys_fs
>> fwts_battery_get_name_proc_fs
>>
>> ..so remove it.
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>   src/lib/src/fwts_battery.c |   16 ++++++++--------
>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/lib/src/fwts_battery.c b/src/lib/src/fwts_battery.c
>> index 656cc33..b792d0b 100644
>> --- a/src/lib/src/fwts_battery.c
>> +++ b/src/lib/src/fwts_battery.c
>> @@ -176,7 +176,7 @@ static int
>> fwts_battery_get_capacity_proc_fs(fwts_framework *fw,
>>       return FWTS_OK;
>>   }
>>
>> -static int fwts_battery_get_count_sys_fs(fwts_framework *fw, DIR
>> *dir, int *count)
>> +static int fwts_battery_get_count_sys_fs(DIR *dir, int *count)
>>   {
>>       struct dirent *entry;
>>       char path[PATH_MAX];
>> @@ -197,7 +197,7 @@ static int
>> fwts_battery_get_count_sys_fs(fwts_framework *fw, DIR *dir, int *coun
>>       return FWTS_OK;
>>   }
>>
>> -static int fwts_battery_get_count_proc_fs(fwts_framework *fw, DIR
>> *dir, int *count)
>> +static int fwts_battery_get_count_proc_fs(DIR *dir, int *count)
>>   {
>>       struct dirent *entry;
>>       do {
>> @@ -208,7 +208,7 @@ static int
>> fwts_battery_get_count_proc_fs(fwts_framework *fw, DIR *dir, int *cou
>>       return FWTS_OK;
>>   }
>>
>> -static int fwts_battery_get_name_sys_fs(fwts_framework *fw, DIR *dir,
>> int index, char *name)
>> +static int fwts_battery_get_name_sys_fs(DIR *dir, int index, char *name)
>>   {
>>       struct dirent *entry;
>>       char path[PATH_MAX];
>> @@ -242,7 +242,7 @@ static int
>> fwts_battery_get_name_sys_fs(fwts_framework *fw, DIR *dir, int index,
>>       return FWTS_ERROR;
>>   }
>>
>> -static int fwts_battery_get_name_proc_fs(fwts_framework *fw, DIR
>> *dir, int index, char *name)
>> +static int fwts_battery_get_name_proc_fs(DIR *dir, int index, char
>> *name)
>>   {
>>       struct dirent *entry;
>>       int i = 0;
>> @@ -591,10 +591,10 @@ int fwts_battery_get_name(fwts_framework *fw,
>> int index, char *name)
>>       DIR *dir;
>>
>>       if ((dir = opendir(FWTS_SYS_CLASS_POWER_SUPPLY)) != NULL) {
>> -        ret = fwts_battery_get_name_sys_fs(fw, dir, index, name);
>> +        ret = fwts_battery_get_name_sys_fs(dir, index, name);
>>           closedir(dir);
>>       } else if ((dir = opendir(FWTS_PROC_ACPI_BATTERY)) != NULL) {
>> -        ret = fwts_battery_get_name_proc_fs(fw, dir, index, name);
>> +        ret = fwts_battery_get_name_proc_fs(dir, index, name);
>>           closedir(dir);
>>       } else {
>>           return FWTS_ERROR;
>> @@ -609,10 +609,10 @@ int fwts_battery_get_count(fwts_framework *fw,
>> int *count)
>>       DIR *dir;
>>
>>       if ((dir = opendir(FWTS_SYS_CLASS_POWER_SUPPLY)) != NULL) {
>> -        ret = fwts_battery_get_count_sys_fs(fw, dir, count);
>> +        ret = fwts_battery_get_count_sys_fs(dir, count);
>>           closedir(dir);
>>       } else if ((dir = opendir(FWTS_PROC_ACPI_BATTERY)) != NULL) {
>> -        ret = fwts_battery_get_count_proc_fs(fw, dir, count);
>> +        ret = fwts_battery_get_count_proc_fs(dir, count);
>>           closedir(dir);
>>       } else {
>>           return FWTS_ERROR;
>>
>
> Functions such as fwts_battery_get_capacity,
> fwts_battery_get_capacity_proc_fs and fwts_battery_get_capacity_sys_fs
> still have the fwts_framework parameter and it enables *fwts_log_info*
> to log.

Indeed - but the helper functions don't need to log anything, so lets 
remove redundant code..

>
> Even though fwts_framework is not used now, will it still be better to
> keep them for consistency? (Removing them and adding them back when
> necessary are just as good. It is just to a second opinion.)


Good question :-)

Since these are internal helper functions and don't change the overall
API then I'm keen to remove any old redundant or unnecessary cruft from 
the code, especially as I'd ultimately like to use the -Wextra flag once 
fwts is really in shape for this as -Wextra can find buggy code.

Colin
>
> Cheers,
> Alex Hung
>
>
>
Alex Hung - Oct. 15, 2012, 8:12 a.m.
On 10/15/2012 04:32 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> We don't require the fwts_framework parameter in the following functions:
>
> fwts_battery_get_count_sys_fs
> fwts_battery_get_count_proc_fs
> fwts_battery_get_name_sys_fs
> fwts_battery_get_name_proc_fs
>
> ..so remove it.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/lib/src/fwts_battery.c |   16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/src/lib/src/fwts_battery.c b/src/lib/src/fwts_battery.c
> index 656cc33..b792d0b 100644
> --- a/src/lib/src/fwts_battery.c
> +++ b/src/lib/src/fwts_battery.c
> @@ -176,7 +176,7 @@ static int fwts_battery_get_capacity_proc_fs(fwts_framework *fw,
>   	return FWTS_OK;
>   }
>
> -static int fwts_battery_get_count_sys_fs(fwts_framework *fw, DIR *dir, int *count)
> +static int fwts_battery_get_count_sys_fs(DIR *dir, int *count)
>   {
>   	struct dirent *entry;
>   	char path[PATH_MAX];
> @@ -197,7 +197,7 @@ static int fwts_battery_get_count_sys_fs(fwts_framework *fw, DIR *dir, int *coun
>   	return FWTS_OK;
>   }
>
> -static int fwts_battery_get_count_proc_fs(fwts_framework *fw, DIR *dir, int *count)
> +static int fwts_battery_get_count_proc_fs(DIR *dir, int *count)
>   {
>   	struct dirent *entry;
>   	do {
> @@ -208,7 +208,7 @@ static int fwts_battery_get_count_proc_fs(fwts_framework *fw, DIR *dir, int *cou
>   	return FWTS_OK;
>   }
>
> -static int fwts_battery_get_name_sys_fs(fwts_framework *fw, DIR *dir, int index, char *name)
> +static int fwts_battery_get_name_sys_fs(DIR *dir, int index, char *name)
>   {
>   	struct dirent *entry;
>   	char path[PATH_MAX];
> @@ -242,7 +242,7 @@ static int fwts_battery_get_name_sys_fs(fwts_framework *fw, DIR *dir, int index,
>   	return FWTS_ERROR;
>   }
>
> -static int fwts_battery_get_name_proc_fs(fwts_framework *fw, DIR *dir, int index, char *name)
> +static int fwts_battery_get_name_proc_fs(DIR *dir, int index, char *name)
>   {
>   	struct dirent *entry;
>   	int i = 0;
> @@ -591,10 +591,10 @@ int fwts_battery_get_name(fwts_framework *fw, int index, char *name)
>   	DIR *dir;
>
>   	if ((dir = opendir(FWTS_SYS_CLASS_POWER_SUPPLY)) != NULL) {
> -		ret = fwts_battery_get_name_sys_fs(fw, dir, index, name);
> +		ret = fwts_battery_get_name_sys_fs(dir, index, name);
>   		closedir(dir);
>   	} else if ((dir = opendir(FWTS_PROC_ACPI_BATTERY)) != NULL) {
> -		ret = fwts_battery_get_name_proc_fs(fw, dir, index, name);
> +		ret = fwts_battery_get_name_proc_fs(dir, index, name);
>   		closedir(dir);
>   	} else {
>   		return FWTS_ERROR;
> @@ -609,10 +609,10 @@ int fwts_battery_get_count(fwts_framework *fw, int *count)
>   	DIR *dir;
>
>   	if ((dir = opendir(FWTS_SYS_CLASS_POWER_SUPPLY)) != NULL) {
> -		ret = fwts_battery_get_count_sys_fs(fw, dir, count);
> +		ret = fwts_battery_get_count_sys_fs(dir, count);
>   		closedir(dir);
>   	} else if ((dir = opendir(FWTS_PROC_ACPI_BATTERY)) != NULL) {
> -		ret = fwts_battery_get_count_proc_fs(fw, dir, count);
> +		ret = fwts_battery_get_count_proc_fs(dir, count);
>   		closedir(dir);
>   	} else {
>   		return FWTS_ERROR;
>

Acked-by: Alex Hung <alex.hung@canonical.com>
Keng-Yu Lin - Oct. 17, 2012, 7:42 a.m.
On Mon, Oct 15, 2012 at 4:32 AM, Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> We don't require the fwts_framework parameter in the following functions:
>
> fwts_battery_get_count_sys_fs
> fwts_battery_get_count_proc_fs
> fwts_battery_get_name_sys_fs
> fwts_battery_get_name_proc_fs
>
> ..so remove it.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/lib/src/fwts_battery.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/src/lib/src/fwts_battery.c b/src/lib/src/fwts_battery.c
> index 656cc33..b792d0b 100644
> --- a/src/lib/src/fwts_battery.c
> +++ b/src/lib/src/fwts_battery.c
> @@ -176,7 +176,7 @@ static int fwts_battery_get_capacity_proc_fs(fwts_framework *fw,
>         return FWTS_OK;
>  }
>
> -static int fwts_battery_get_count_sys_fs(fwts_framework *fw, DIR *dir, int *count)
> +static int fwts_battery_get_count_sys_fs(DIR *dir, int *count)
>  {
>         struct dirent *entry;
>         char path[PATH_MAX];
> @@ -197,7 +197,7 @@ static int fwts_battery_get_count_sys_fs(fwts_framework *fw, DIR *dir, int *coun
>         return FWTS_OK;
>  }
>
> -static int fwts_battery_get_count_proc_fs(fwts_framework *fw, DIR *dir, int *count)
> +static int fwts_battery_get_count_proc_fs(DIR *dir, int *count)
>  {
>         struct dirent *entry;
>         do {
> @@ -208,7 +208,7 @@ static int fwts_battery_get_count_proc_fs(fwts_framework *fw, DIR *dir, int *cou
>         return FWTS_OK;
>  }
>
> -static int fwts_battery_get_name_sys_fs(fwts_framework *fw, DIR *dir, int index, char *name)
> +static int fwts_battery_get_name_sys_fs(DIR *dir, int index, char *name)
>  {
>         struct dirent *entry;
>         char path[PATH_MAX];
> @@ -242,7 +242,7 @@ static int fwts_battery_get_name_sys_fs(fwts_framework *fw, DIR *dir, int index,
>         return FWTS_ERROR;
>  }
>
> -static int fwts_battery_get_name_proc_fs(fwts_framework *fw, DIR *dir, int index, char *name)
> +static int fwts_battery_get_name_proc_fs(DIR *dir, int index, char *name)
>  {
>         struct dirent *entry;
>         int i = 0;
> @@ -591,10 +591,10 @@ int fwts_battery_get_name(fwts_framework *fw, int index, char *name)
>         DIR *dir;
>
>         if ((dir = opendir(FWTS_SYS_CLASS_POWER_SUPPLY)) != NULL) {
> -               ret = fwts_battery_get_name_sys_fs(fw, dir, index, name);
> +               ret = fwts_battery_get_name_sys_fs(dir, index, name);
>                 closedir(dir);
>         } else if ((dir = opendir(FWTS_PROC_ACPI_BATTERY)) != NULL) {
> -               ret = fwts_battery_get_name_proc_fs(fw, dir, index, name);
> +               ret = fwts_battery_get_name_proc_fs(dir, index, name);
>                 closedir(dir);
>         } else {
>                 return FWTS_ERROR;
> @@ -609,10 +609,10 @@ int fwts_battery_get_count(fwts_framework *fw, int *count)
>         DIR *dir;
>
>         if ((dir = opendir(FWTS_SYS_CLASS_POWER_SUPPLY)) != NULL) {
> -               ret = fwts_battery_get_count_sys_fs(fw, dir, count);
> +               ret = fwts_battery_get_count_sys_fs(dir, count);
>                 closedir(dir);
>         } else if ((dir = opendir(FWTS_PROC_ACPI_BATTERY)) != NULL) {
> -               ret = fwts_battery_get_count_proc_fs(fw, dir, count);
> +               ret = fwts_battery_get_count_proc_fs(dir, count);
>                 closedir(dir);
>         } else {
>                 return FWTS_ERROR;
> --
> 1.7.10.4
>
Acked-by: Keng-Yu Lin <kengyu@canonical.com>

Patch

diff --git a/src/lib/src/fwts_battery.c b/src/lib/src/fwts_battery.c
index 656cc33..b792d0b 100644
--- a/src/lib/src/fwts_battery.c
+++ b/src/lib/src/fwts_battery.c
@@ -176,7 +176,7 @@  static int fwts_battery_get_capacity_proc_fs(fwts_framework *fw,
 	return FWTS_OK;
 }
 
-static int fwts_battery_get_count_sys_fs(fwts_framework *fw, DIR *dir, int *count)
+static int fwts_battery_get_count_sys_fs(DIR *dir, int *count)
 {
 	struct dirent *entry;
 	char path[PATH_MAX];
@@ -197,7 +197,7 @@  static int fwts_battery_get_count_sys_fs(fwts_framework *fw, DIR *dir, int *coun
 	return FWTS_OK;
 }
 
-static int fwts_battery_get_count_proc_fs(fwts_framework *fw, DIR *dir, int *count)
+static int fwts_battery_get_count_proc_fs(DIR *dir, int *count)
 {
 	struct dirent *entry;
 	do {
@@ -208,7 +208,7 @@  static int fwts_battery_get_count_proc_fs(fwts_framework *fw, DIR *dir, int *cou
 	return FWTS_OK;
 }
 
-static int fwts_battery_get_name_sys_fs(fwts_framework *fw, DIR *dir, int index, char *name)
+static int fwts_battery_get_name_sys_fs(DIR *dir, int index, char *name)
 {
 	struct dirent *entry;
 	char path[PATH_MAX];
@@ -242,7 +242,7 @@  static int fwts_battery_get_name_sys_fs(fwts_framework *fw, DIR *dir, int index,
 	return FWTS_ERROR;
 }
 
-static int fwts_battery_get_name_proc_fs(fwts_framework *fw, DIR *dir, int index, char *name)
+static int fwts_battery_get_name_proc_fs(DIR *dir, int index, char *name)
 {
 	struct dirent *entry;
 	int i = 0;
@@ -591,10 +591,10 @@  int fwts_battery_get_name(fwts_framework *fw, int index, char *name)
 	DIR *dir;
 
 	if ((dir = opendir(FWTS_SYS_CLASS_POWER_SUPPLY)) != NULL) {
-		ret = fwts_battery_get_name_sys_fs(fw, dir, index, name);
+		ret = fwts_battery_get_name_sys_fs(dir, index, name);
 		closedir(dir);
 	} else if ((dir = opendir(FWTS_PROC_ACPI_BATTERY)) != NULL) {
-		ret = fwts_battery_get_name_proc_fs(fw, dir, index, name);
+		ret = fwts_battery_get_name_proc_fs(dir, index, name);
 		closedir(dir);
 	} else {
 		return FWTS_ERROR;
@@ -609,10 +609,10 @@  int fwts_battery_get_count(fwts_framework *fw, int *count)
 	DIR *dir;
 
 	if ((dir = opendir(FWTS_SYS_CLASS_POWER_SUPPLY)) != NULL) {
-		ret = fwts_battery_get_count_sys_fs(fw, dir, count);
+		ret = fwts_battery_get_count_sys_fs(dir, count);
 		closedir(dir);
 	} else if ((dir = opendir(FWTS_PROC_ACPI_BATTERY)) != NULL) {
-		ret = fwts_battery_get_count_proc_fs(fw, dir, count);
+		ret = fwts_battery_get_count_proc_fs(dir, count);
 		closedir(dir);
 	} else {
 		return FWTS_ERROR;