diff mbox

lib: fwts_release: call lsb_release rather than parsing raw data (LP: #1266823)

Message ID 1389109266-4050-1-git-send-email-colin.king@canonical.com
State Accepted
Headers show

Commit Message

Colin Ian King Jan. 7, 2014, 3:41 p.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

Getting release info only works if /etc/lsb_release exists. Some Debian
based systems may have this information fetched from other places, so instead
parse the output from lsb_release -a to get the information as this provides
the consistent output across Debian systems.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/lib/src/fwts_release.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

Comments

Alex Hung Jan. 8, 2014, 7:43 a.m. UTC | #1 
On 01/07/2014 11:41 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Getting release info only works if /etc/lsb_release exists. Some Debian
> based systems may have this information fetched from other places, so instead
> parse the output from lsb_release -a to get the information as this provides
> the consistent output across Debian systems.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/lib/src/fwts_release.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/src/lib/src/fwts_release.c b/src/lib/src/fwts_release.c
> index 24f8789..f82ab4e 100644
> --- a/src/lib/src/fwts_release.c
> +++ b/src/lib/src/fwts_release.c
> @@ -35,7 +35,7 @@ static void fwts_release_field_get(char *needle, char *delimiter, char *text, ch
>   	if (strstr(text, needle) == NULL)
>   		return;
>   	str++;
> -	while (*str == ' ')
> +	while (*str == ' ' || *str == '\t')
>   		str++;
>
>   	if (*str)
> @@ -53,10 +53,10 @@ void fwts_release_get_debian(fwts_list *list, fwts_release *release)
>   	fwts_list_foreach(item, list) {
>   		char *line = fwts_text_list_text(item);
>
> -		fwts_release_field_get("DISTRIB_ID", "=", line, &release->distributor);
> -		fwts_release_field_get("DISTRIB_RELEASE", "=", line, &release->description);
> -		fwts_release_field_get("DISTRIB_CODENAME", "=", line, &release->release);
> -		fwts_release_field_get("DISTRIB_DESCRIPTION", "=", line, &release->codename);
> +		fwts_release_field_get("Distributor ID:", ":", line, &release->distributor);
> +		fwts_release_field_get("Release", ":", line, &release->description);
> +		fwts_release_field_get("Codename", ":", line, &release->release);
> +		fwts_release_field_get("Description", ":", line, &release->codename);
>   	}
>   }
>
> @@ -78,6 +78,7 @@ fwts_release *fwts_release_get(void)
>   {
>   	fwts_list *list;
>   	fwts_release *release;
> +	int status;
>
>   	if ((release = calloc(1, sizeof(fwts_release))) == NULL)
>   		return NULL;
> @@ -86,7 +87,11 @@ fwts_release *fwts_release_get(void)
>   	 *  For the moment, check in /etc/lsb-release, we need to add in
>   	 *  support for different distros too.
>   	 */
> -	if ((list = fwts_file_open_and_read("/etc/lsb-release")) != NULL) {
> +	if (fwts_pipe_exec("lsb_release -a", &list, &status) != FWTS_OK) {
> +		free(release);
> +		return NULL;
> +	}
> +	if (list) {
>   		fwts_release_get_debian(list, release);
>   		fwts_list_free(list, free);
>   	}
>

Acked-by: Alex Hung <alex.hung@canonical.com>
Ivan Hu Jan. 9, 2014, 6:49 a.m. UTC | #2 
On 01/07/2014 11:41 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Getting release info only works if /etc/lsb_release exists. Some Debian
> based systems may have this information fetched from other places, so instead
> parse the output from lsb_release -a to get the information as this provides
> the consistent output across Debian systems.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/lib/src/fwts_release.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/src/lib/src/fwts_release.c b/src/lib/src/fwts_release.c
> index 24f8789..f82ab4e 100644
> --- a/src/lib/src/fwts_release.c
> +++ b/src/lib/src/fwts_release.c
> @@ -35,7 +35,7 @@ static void fwts_release_field_get(char *needle, char *delimiter, char *text, ch
>   	if (strstr(text, needle) == NULL)
>   		return;
>   	str++;
> -	while (*str == ' ')
> +	while (*str == ' ' || *str == '\t')
>   		str++;
>
>   	if (*str)
> @@ -53,10 +53,10 @@ void fwts_release_get_debian(fwts_list *list, fwts_release *release)
>   	fwts_list_foreach(item, list) {
>   		char *line = fwts_text_list_text(item);
>
> -		fwts_release_field_get("DISTRIB_ID", "=", line, &release->distributor);
> -		fwts_release_field_get("DISTRIB_RELEASE", "=", line, &release->description);
> -		fwts_release_field_get("DISTRIB_CODENAME", "=", line, &release->release);
> -		fwts_release_field_get("DISTRIB_DESCRIPTION", "=", line, &release->codename);
> +		fwts_release_field_get("Distributor ID:", ":", line, &release->distributor);

Just minor thing,
The Field should be changed to "Distributor ID", remove the delimiter.

> +		fwts_release_field_get("Release", ":", line, &release->description);
> +		fwts_release_field_get("Codename", ":", line, &release->release);
> +		fwts_release_field_get("Description", ":", line, &release->codename);
>   	}
>   }
>
> @@ -78,6 +78,7 @@ fwts_release *fwts_release_get(void)
>   {
>   	fwts_list *list;
>   	fwts_release *release;
> +	int status;
>
>   	if ((release = calloc(1, sizeof(fwts_release))) == NULL)
>   		return NULL;
> @@ -86,7 +87,11 @@ fwts_release *fwts_release_get(void)
>   	 *  For the moment, check in /etc/lsb-release, we need to add in
>   	 *  support for different distros too.
>   	 */
> -	if ((list = fwts_file_open_and_read("/etc/lsb-release")) != NULL) {
> +	if (fwts_pipe_exec("lsb_release -a", &list, &status) != FWTS_OK) {
> +		free(release);
> +		return NULL;
> +	}
> +	if (list) {
>   		fwts_release_get_debian(list, release);
>   		fwts_list_free(list, free);
>   	}
>
Keng-Yu Lin Jan. 14, 2014, 7:36 a.m. UTC | #3 
On Thu, Jan 9, 2014 at 2:49 PM, IvanHu <ivan.hu@canonical.com> wrote:
> On 01/07/2014 11:41 PM, Colin King wrote:
>>
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Getting release info only works if /etc/lsb_release exists. Some Debian
>> based systems may have this information fetched from other places, so
>> instead
>> parse the output from lsb_release -a to get the information as this
>> provides
>> the consistent output across Debian systems.
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>   src/lib/src/fwts_release.c | 17 +++++++++++------
>>   1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/lib/src/fwts_release.c b/src/lib/src/fwts_release.c
>> index 24f8789..f82ab4e 100644
>> --- a/src/lib/src/fwts_release.c
>> +++ b/src/lib/src/fwts_release.c
>> @@ -35,7 +35,7 @@ static void fwts_release_field_get(char *needle, char
>> *delimiter, char *text, ch
>>         if (strstr(text, needle) == NULL)
>>                 return;
>>         str++;
>> -       while (*str == ' ')
>> +       while (*str == ' ' || *str == '\t')
>>                 str++;
>>
>>         if (*str)
>> @@ -53,10 +53,10 @@ void fwts_release_get_debian(fwts_list *list,
>> fwts_release *release)
>>         fwts_list_foreach(item, list) {
>>                 char *line = fwts_text_list_text(item);
>>
>> -               fwts_release_field_get("DISTRIB_ID", "=", line,
>> &release->distributor);
>> -               fwts_release_field_get("DISTRIB_RELEASE", "=", line,
>> &release->description);
>> -               fwts_release_field_get("DISTRIB_CODENAME", "=", line,
>> &release->release);
>> -               fwts_release_field_get("DISTRIB_DESCRIPTION", "=", line,
>> &release->codename);
>> +               fwts_release_field_get("Distributor ID:", ":", line,
>> &release->distributor);
>
>
> Just minor thing,
> The Field should be changed to "Distributor ID", remove the delimiter.
>
>
>> +               fwts_release_field_get("Release", ":", line,
>> &release->description);
>> +               fwts_release_field_get("Codename", ":", line,
>> &release->release);
>> +               fwts_release_field_get("Description", ":", line,
>> &release->codename);
>>         }
>>   }
>>
>> @@ -78,6 +78,7 @@ fwts_release *fwts_release_get(void)
>>   {
>>         fwts_list *list;
>>         fwts_release *release;
>> +       int status;
>>
>>         if ((release = calloc(1, sizeof(fwts_release))) == NULL)
>>                 return NULL;
>> @@ -86,7 +87,11 @@ fwts_release *fwts_release_get(void)
>>          *  For the moment, check in /etc/lsb-release, we need to add in
>>          *  support for different distros too.
>>          */
>> -       if ((list = fwts_file_open_and_read("/etc/lsb-release")) != NULL)
>> {
>> +       if (fwts_pipe_exec("lsb_release -a", &list, &status) != FWTS_OK) {
>> +               free(release);
>> +               return NULL;
>> +       }
>> +       if (list) {
>>                 fwts_release_get_debian(list, release);
>>                 fwts_list_free(list, free);
>>         }
>>
>
>

Acked-by: Keng-Yu Lin <kengyu@canonical.com>
Colin Ian King Feb. 14, 2014, 9:33 a.m. UTC | #4 
Hi there,

this patch was ack'd a while ago and has not been applied to the git
repo. Can it be applied at some point? Thanks.

On 14/01/14 07:36, Keng-Yu Lin wrote:
> On Thu, Jan 9, 2014 at 2:49 PM, IvanHu <ivan.hu@canonical.com> wrote:
>> On 01/07/2014 11:41 PM, Colin King wrote:
>>>
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> Getting release info only works if /etc/lsb_release exists. Some Debian
>>> based systems may have this information fetched from other places, so
>>> instead
>>> parse the output from lsb_release -a to get the information as this
>>> provides
>>> the consistent output across Debian systems.
>>>
>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>> ---
>>>   src/lib/src/fwts_release.c | 17 +++++++++++------
>>>   1 file changed, 11 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/lib/src/fwts_release.c b/src/lib/src/fwts_release.c
>>> index 24f8789..f82ab4e 100644
>>> --- a/src/lib/src/fwts_release.c
>>> +++ b/src/lib/src/fwts_release.c
>>> @@ -35,7 +35,7 @@ static void fwts_release_field_get(char *needle, char
>>> *delimiter, char *text, ch
>>>         if (strstr(text, needle) == NULL)
>>>                 return;
>>>         str++;
>>> -       while (*str == ' ')
>>> +       while (*str == ' ' || *str == '\t')
>>>                 str++;
>>>
>>>         if (*str)
>>> @@ -53,10 +53,10 @@ void fwts_release_get_debian(fwts_list *list,
>>> fwts_release *release)
>>>         fwts_list_foreach(item, list) {
>>>                 char *line = fwts_text_list_text(item);
>>>
>>> -               fwts_release_field_get("DISTRIB_ID", "=", line,
>>> &release->distributor);
>>> -               fwts_release_field_get("DISTRIB_RELEASE", "=", line,
>>> &release->description);
>>> -               fwts_release_field_get("DISTRIB_CODENAME", "=", line,
>>> &release->release);
>>> -               fwts_release_field_get("DISTRIB_DESCRIPTION", "=", line,
>>> &release->codename);
>>> +               fwts_release_field_get("Distributor ID:", ":", line,
>>> &release->distributor);
>>
>>
>> Just minor thing,
>> The Field should be changed to "Distributor ID", remove the delimiter.
>>
>>
>>> +               fwts_release_field_get("Release", ":", line,
>>> &release->description);
>>> +               fwts_release_field_get("Codename", ":", line,
>>> &release->release);
>>> +               fwts_release_field_get("Description", ":", line,
>>> &release->codename);
>>>         }
>>>   }
>>>
>>> @@ -78,6 +78,7 @@ fwts_release *fwts_release_get(void)
>>>   {
>>>         fwts_list *list;
>>>         fwts_release *release;
>>> +       int status;
>>>
>>>         if ((release = calloc(1, sizeof(fwts_release))) == NULL)
>>>                 return NULL;
>>> @@ -86,7 +87,11 @@ fwts_release *fwts_release_get(void)
>>>          *  For the moment, check in /etc/lsb-release, we need to add in
>>>          *  support for different distros too.
>>>          */
>>> -       if ((list = fwts_file_open_and_read("/etc/lsb-release")) != NULL)
>>> {
>>> +       if (fwts_pipe_exec("lsb_release -a", &list, &status) != FWTS_OK) {
>>> +               free(release);
>>> +               return NULL;
>>> +       }
>>> +       if (list) {
>>>                 fwts_release_get_debian(list, release);
>>>                 fwts_list_free(list, free);
>>>         }
>>>
>>
>>
> 
> Acked-by: Keng-Yu Lin <kengyu@canonical.com>
>
diff mbox

Patch

diff --git a/src/lib/src/fwts_release.c b/src/lib/src/fwts_release.c
index 24f8789..f82ab4e 100644
--- a/src/lib/src/fwts_release.c
+++ b/src/lib/src/fwts_release.c
@@ -35,7 +35,7 @@  static void fwts_release_field_get(char *needle, char *delimiter, char *text, ch
 	if (strstr(text, needle) == NULL)
 		return;
 	str++;
-	while (*str == ' ')
+	while (*str == ' ' || *str == '\t')
 		str++;
 
 	if (*str)
@@ -53,10 +53,10 @@  void fwts_release_get_debian(fwts_list *list, fwts_release *release)
 	fwts_list_foreach(item, list) {
 		char *line = fwts_text_list_text(item);
 
-		fwts_release_field_get("DISTRIB_ID", "=", line, &release->distributor);
-		fwts_release_field_get("DISTRIB_RELEASE", "=", line, &release->description);
-		fwts_release_field_get("DISTRIB_CODENAME", "=", line, &release->release);
-		fwts_release_field_get("DISTRIB_DESCRIPTION", "=", line, &release->codename);
+		fwts_release_field_get("Distributor ID:", ":", line, &release->distributor);
+		fwts_release_field_get("Release", ":", line, &release->description);
+		fwts_release_field_get("Codename", ":", line, &release->release);
+		fwts_release_field_get("Description", ":", line, &release->codename);
 	}
 }
 
@@ -78,6 +78,7 @@  fwts_release *fwts_release_get(void)
 {
 	fwts_list *list;
 	fwts_release *release;
+	int status;
 
 	if ((release = calloc(1, sizeof(fwts_release))) == NULL)
 		return NULL;
@@ -86,7 +87,11 @@  fwts_release *fwts_release_get(void)
 	 *  For the moment, check in /etc/lsb-release, we need to add in
 	 *  support for different distros too.
 	 */
-	if ((list = fwts_file_open_and_read("/etc/lsb-release")) != NULL) {
+	if (fwts_pipe_exec("lsb_release -a", &list, &status) != FWTS_OK) {
+		free(release);
+		return NULL;
+	}
+	if (list) {
 		fwts_release_get_debian(list, release);
 		fwts_list_free(list, free);
 	}