RFR: os_linux.cpp parse_os_info gives non descriptive output on current SLES releases

[Baesken, Matthias]

?  Thank you for finding this problem with SuSE and the diff.  I don't know

?  how many different distros were tested with these changes but it's easy

?  not to get them all.

?  Would it make more sense, rather than getting the last line then

?  checking for DISTRIB_DESCRIPTION=, checking in the fgets() loop for both

?  this and PRETTY_NAME.  Then we could change it for Ubuntu and there's

?  less conditionalizing on the name of the release file.

?  Like:

?  open webrev at http://cr.openjdk.java.net/~coleenp/suse/webrev

?

?  Coleen



Hello Coleen,  thanks a lot for creating the webrev,  I think it looks good (better than original diff)    with the exception of

the indentation at  the beginning  of   parse_os_info_helper  (buf declaration) .



2065 static void parse_os_info_helper(FILE* fp, char* distro, size_t length, bool get_first_line) {

2066     char buf[256];

2067   while (fgets(buf, sizeof(buf), fp)) {





Best regards, Matthias





  --------------------------------------------------------------

On 4/18/16 8:05 AM, Baesken, Matthias wrote:

> Hello ,  the current implementation of the  parse_os_info-function in os_linux.cpp  gets the last line of a Linux-distro related file to

> provide a meaningful OS version string.

>

> However the information provided currently on SuSE Linux (SLES) is not very descriptive, it currently uses /etc/lsb-release and gives :

>

> more /etc/lsb-release

> LSB_VERSION="core-2.0-noarch:core-3.2-noarch:core-4.0-noarch:core-2.0-x86_64:core-3.2-x86_64:core-4.0-x86_64"

>

> So I suggest to use /etc/SuSE-release instead, which gives a good information for

> SLES 9 - 12 in the ***first line*** of  /etc/SuSE-release :

>

> Example SLES11 :

>

> more /etc/SuSE-release

> SUSE Linux Enterprise Server 11 (x86_64)

> VERSION = 11

> PATCHLEVEL = 3

>

> (this is similar to using /etc/redhat-release on Red Hat with the difference that the ***first line*** has the relevant info).

>

>

> Additionally, /etc/os-release needs some special handling as well, because

> the meaningful OS-release description string is not always  the last line of the file but in the line

> containing the information PRETTY_NAME=...

> See also :

>

> https://www.freedesktop.org/software/systemd/man/os-release.html

>

> Example from Ubuntu 14 :

>

> $ more /etc/os-release

>    ...

> PRETTY_NAME="Ubuntu 14.04.3 LTS"

>    ...

>

> It might also be a good idea to place  /etc/os-release  higher in the distro_files list, but I do not have access to

> turbolinux / gentoo to check the situation on these distros.

>

> Regards, Matthias

>

>

>

> Diff :

> --- a/src/os/linux/vm/os_linux.cpp      Fri Apr 15 16:19:15 2016 +0100

> +++ b/src/os/linux/vm/os_linux.cpp      Mon Apr 18 13:54:04 2016 +0200

> @@ -2013,8 +2013,8 @@

> // their own specific XXX-release file as well as a redhat-release file.

> // Because of this the XXX-release file needs to be searched for before the

> // redhat-release file.

> -// Since Red Hat has a lsb-release file that is not very descriptive the

> -// search for redhat-release needs to be before lsb-release.

> +// Since Red Hat and SuSE have an lsb-release file that is not very descriptive the

> +// search for redhat-release / SuSE-release needs to be before lsb-release.

> // Since the lsb-release file is the new standard it needs to be searched

> // before the older style release files.

> // Searching system-release (Red Hat) and os-release (other Linuxes) are a

> @@ -2031,8 +2031,8 @@

>     "/etc/mandrake-release",

>     "/etc/sun-release",

>     "/etc/redhat-release",

> +  "/etc/SuSE-release",

>     "/etc/lsb-release",

> -  "/etc/SuSE-release",

>     "/etc/turbolinux-release",

>     "/etc/gentoo-release",

>     "/etc/ltib-release",

> @@ -2065,11 +2065,36 @@

> static void parse_os_info(char* distro, size_t length, const char* file) {

>     FILE* fp = fopen(file, "r");

>     if (fp != NULL) {

> +    // SuSE-release : first line is interesting

> +    // os-release : PRETTY_NAME= line is interesting

> +    // (might be at different locations in the file)

>       char buf[256];

> -    // get last line of the file.

> -    while (fgets(buf, sizeof(buf), fp)) { }

> +    int lcnt = 0;

> +    bool is_etc_suserelease = false;

> +    bool is_etc_osrelease   = false;

> +    if (strcmp(file, "/etc/SuSE-release") == 0) {

> +      is_etc_suserelease = true;

> +    }

> +    if (strcmp(file, "/etc/os-release") == 0) {

> +      is_etc_osrelease = true;

> +    }

> +

> +    // get last line of the file or

> +    // other interesting line on SUSE / os-release

> +    while (fgets(buf, sizeof(buf), fp)) {

> +      if (lcnt == 0 && is_etc_suserelease) {

> +        break;

> +      }

> +      if (is_etc_osrelease) {

> +        if (strstr(buf, "PRETTY_NAME=") != NULL) {

> +          break;

> +        }

> +      }

> +      lcnt++;

> +    }

> +

>       // Edit out extra stuff in expected ubuntu format

> -    if (strstr(buf, "DISTRIB_DESCRIPTION=") != NULL) {

> +    if (strstr(buf, "DISTRIB_DESCRIPTION=") != NULL || strstr(buf, "PRETTY_NAME=") != NULL) {

>         char* ptr = strstr(buf, "\"");  // the name is in quotes

>         if (ptr != NULL) {

>           ptr++; // go beyond first quote