[Buildroot] [PATCH] package/lshw: fix musl build

[Buildroot] [PATCH] package/lshw: fix musl build

[Romain Naour]

The upstream commit adding musl support was not enough to build lshw
with musl.

dasd.cc: In function 'bool scan_dasd(hwNode&)':
dasd.cc:45:52: error: 'basename' was not declared in this scope
       dev_name = basename(devices.gl_pathv[dev_num]);

Use the same fix.

Upstream status: pending
https://github.com/lyonel/lshw/pull/17

Fixes:
http://autobuild.buildroot.net/results/aa1/aa1c20866fda025167b0d220b316c7d85a1b9663

Signed-off-by: Romain Naour <romain.naour@gmail.com>
---
 package/lshw/0002-dasd-sysfs-fix-musl-build.patch | 95 +++++++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 package/lshw/0002-dasd-sysfs-fix-musl-build.patch

diff --git a/package/lshw/0002-dasd-sysfs-fix-musl-build.patch b/package/lshw/0002-dasd-sysfs-fix-musl-build.patch
new file mode 100644
index 0000000..c35d83b
--- /dev/null
+++ b/package/lshw/0002-dasd-sysfs-fix-musl-build.patch
@@ -0,0 +1,95 @@
+From 8245b8812ee7729927a7755e9c1f69f089182fac Mon Sep 17 00:00:00 2001
+From: Romain Naour <romain.naour@gmail.com>
+Date: Thu, 4 Aug 2016 17:58:04 +0200
+Subject: [PATCH] dasd,sysfs: fix musl build
+
+The commit cd690bff1516b40fecd5ec4a7f6619e5bffc3cf0 adding musl support
+was not enough to build lshw with musl.
+
+dasd.cc: In function 'bool scan_dasd(hwNode&)':
+dasd.cc:45:52: error: 'basename' was not declared in this scope
+       dev_name = basename(devices.gl_pathv[dev_num]);
+
+Use the same fix.
+
+Fixes:
+http://autobuild.buildroot.net/results/aa1/aa1c20866fda025167b0d220b316c7d85a1b9663
+
+Signed-off-by: Romain Naour <romain.naour@gmail.com>
+---
+ src/core/dasd.cc  | 3 ++-
+ src/core/sysfs.cc | 9 +++++----
+ 2 files changed, 7 insertions(+), 5 deletions(-)
+
+diff --git a/src/core/dasd.cc b/src/core/dasd.cc
+index 626b8a8..18d19c3 100644
+--- a/src/core/dasd.cc
++++ b/src/core/dasd.cc
+@@ -4,6 +4,7 @@
+ #include <glob.h>
+ #include <string.h>
+ #include <fcntl.h>
++#include <libgen.h>
+ #include <unistd.h>
+ #include <inttypes.h>
+ #include <sys/ioctl.h>
+@@ -42,7 +43,7 @@ bool scan_dasd(hwNode & n)
+   {
+     for(dev_num=0;dev_num<devices.gl_pathc;dev_num++)
+     {
+-      dev_name = basename(devices.gl_pathv[dev_num]);
++      dev_name = basename(const_cast<char *>(devices.gl_pathv[dev_num]));
+       for (std::vector<std::string>::iterator it = sysfs_attribs.begin(); it != sysfs_attribs.end(); ++it)
+       {
+         std::string attrib_fname = std::string(SYSFS_PREFIX) + dev_name + "/device/" + *it;
+diff --git a/src/core/sysfs.cc b/src/core/sysfs.cc
+index acc9d00..ad55d11 100644
+--- a/src/core/sysfs.cc
++++ b/src/core/sysfs.cc
+@@ -8,6 +8,7 @@
+ #include "sysfs.h"
+ #include "osutils.h"
+ #include <limits.h>
++#include <libgen.h>
+ #include <unistd.h>
+ #include <stdlib.h>
+ #include <stdio.h>
+@@ -99,7 +100,7 @@ static string sysfs_getbustype(const string & path)
+   {
+     devname =
+       string(fs.path + "/bus/") + string(namelist[i]->d_name) +
+-      "/devices/" + basename(path.c_str());
++      "/devices/" + basename(const_cast<char *>(path.c_str()));
+ 
+     if (samefile(devname, path))
+       return string(namelist[i]->d_name);
+@@ -139,7 +140,7 @@ static string sysfstobusinfo(const string & path)
+ 
+   if (bustype == "virtio")
+   {
+-    string name = basename(path.c_str());
++    string name = basename(const_cast<char *>(path.c_str()));
+     if (name.compare(0, 6, "virtio") == 0)
+       return "virtio@" + name.substr(6);
+     else
+@@ -207,7 +208,7 @@ string entry::driver() const
+   string driverlink = This->devpath + "/driver";
+   if (!exists(driverlink))
+     return "";
+-  return basename(readlink(driverlink).c_str());
++  return basename(const_cast<char *>(readlink(driverlink).c_str()));
+ }
+ 
+ 
+@@ -288,7 +289,7 @@ string entry::name_in_class(const string & classname) const
+ 
+ string entry::name() const
+ {
+-  return basename(This->devpath.c_str());
++  return basename(const_cast<char *>(This->devpath.c_str()));
+ }
+ 
+ 
+-- 
+2.5.5
+
-- 
2.5.5

[Buildroot] [PATCH] package/lshw: fix musl build

[Thomas Petazzoni]

Hello,

On Thu,  4 Aug 2016 18:24:46 +0200, Romain Naour wrote:

> +diff --git a/src/core/dasd.cc b/src/core/dasd.cc
> +index 626b8a8..18d19c3 100644
> +--- a/src/core/dasd.cc
> ++++ b/src/core/dasd.cc
> +@@ -4,6 +4,7 @@
> + #include <glob.h>
> + #include <string.h>
> + #include <fcntl.h>
> ++#include <libgen.h>

Is this related?

> + #include <unistd.h>
> + #include <inttypes.h>
> + #include <sys/ioctl.h>
> +@@ -42,7 +43,7 @@ bool scan_dasd(hwNode & n)
> +   {
> +     for(dev_num=0;dev_num<devices.gl_pathc;dev_num++)
> +     {
> +-      dev_name = basename(devices.gl_pathv[dev_num]);
> ++      dev_name = basename(const_cast<char *>(devices.gl_pathv[dev_num]));

I'm not super familiar with C++ stuff, but why is this problem musl
specific? The basename() function is "char *basename(char *)"
regardless of the C library being used. What makes it error out with
musl and not with other C libraries?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] package/lshw: fix musl build

[Romain Naour]

Hi Thomas,

Le 04/08/2016 ? 18:37, Thomas Petazzoni a ?crit :
> Hello,
> 
> On Thu,  4 Aug 2016 18:24:46 +0200, Romain Naour wrote:
> 
>> +diff --git a/src/core/dasd.cc b/src/core/dasd.cc
>> +index 626b8a8..18d19c3 100644
>> +--- a/src/core/dasd.cc
>> ++++ b/src/core/dasd.cc
>> +@@ -4,6 +4,7 @@
>> + #include <glob.h>
>> + #include <string.h>
>> + #include <fcntl.h>
>> ++#include <libgen.h>
> 
> Is this related?

Yes it is.

Upstream seems to use the POSIX variant of basename which require libgen.h

https://github.com/lyonel/lshw/commit/cd690bff1516b40fecd5ec4a7f6619e5bffc3cf0

> 
>> + #include <unistd.h>
>> + #include <inttypes.h>
>> + #include <sys/ioctl.h>
>> +@@ -42,7 +43,7 @@ bool scan_dasd(hwNode & n)
>> +   {
>> +     for(dev_num=0;dev_num<devices.gl_pathc;dev_num++)
>> +     {
>> +-      dev_name = basename(devices.gl_pathv[dev_num]);
>> ++      dev_name = basename(const_cast<char *>(devices.gl_pathv[dev_num]));
> 
> I'm not super familiar with C++ stuff, but why is this problem musl
> specific? The basename() function is "char *basename(char *)"
> regardless of the C library being used. What makes it error out with
> musl and not with other C libraries?

Actually, I get an error on basename from sysfs.cc

sysfs.cc: In function ?std::string sysfs_getbustype(const string&)?:
sysfs.cc:103:41: error: invalid conversion from ?const char*? to ?char*?
[-fpermissive]
       "/devices/" + basename(path.c_str());

The cast in dasd is not necessary.

Best regards,
Romain

> 
> Thanks,
> 
> Thomas
> 

[Buildroot] [PATCH] package/lshw: fix musl build

[Thomas Petazzoni]

Hello,

On Thu, 4 Aug 2016 19:09:25 +0200, Romain Naour wrote:

> Actually, I get an error on basename from sysfs.cc
> 
> sysfs.cc: In function ?std::string sysfs_getbustype(const string&)?:
> sysfs.cc:103:41: error: invalid conversion from ?const char*? to ?char*?
> [-fpermissive]
>        "/devices/" + basename(path.c_str());
> 
> The cast in dasd is not necessary.

Are you sure it's musl related, and not gcc version related? A
-fpermissive error typically indicates that the compiler has become
stricter than it used to be.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] package/lshw: fix musl build

[Romain Naour]

Hi Thomas,

Le 04/08/2016 ? 20:09, Thomas Petazzoni a ?crit :
> Hello,
> 
> On Thu, 4 Aug 2016 19:09:25 +0200, Romain Naour wrote:
> 
>> Actually, I get an error on basename from sysfs.cc
>>
>> sysfs.cc: In function ?std::string sysfs_getbustype(const string&)?:
>> sysfs.cc:103:41: error: invalid conversion from ?const char*? to ?char*?
>> [-fpermissive]
>>        "/devices/" + basename(path.c_str());
>>
>> The cast in dasd is not necessary.
> 
> Are you sure it's musl related, and not gcc version related? A
> -fpermissive error typically indicates that the compiler has become
> stricter than it used to be.

It seems related to musl. lshw build fine with a toolchain with gcc 5.

I'm not a glibc expert but it seems that glibc accept a char * or const char *
as filename argument, see string.h from glibc:

#  ifdef __CORRECT_ISO_CPP_STRING_H_PROTO
extern "C++" char *basename (char *__filename)
     __THROW __asm ("basename") __nonnull ((1));
extern "C++" const char *basename (const char *__filename)
     __THROW __asm ("basename") __nonnull ((1));
#  else
extern char *basename (const char *__filename) __THROW __nonnull ((1));
#  endif
# endif

The musl version doesn't support the const char * prototype.

Also, the lshw code expect the GNU version of basename(). But this version is
not available from string.h when C++ is used.

See string.h from musl:
#ifndef __cplusplus
char *basename();
#endif

That's the issue reported by autobuilders, so the only way is to use the POSIX
basename() if we want to use lshw with musl.

Upstream fixed a similar issue in src/core/pci.cc using the POSIX basename() [1]
and a cast from const char * to char * to avoid invalid conversion.

Thought ?

Best regards,
Romain

[1] https://github.com/lyonel/lshw/commit/cd690bff1516b40fecd5ec4a7f6619e5bffc3cf0

> 
> Thomas
> 

[Buildroot] [PATCH] package/lshw: fix musl build

[Khem Raj]

On 8/4/16 9:37 AM, Thomas Petazzoni wrote:
> Hello,
> 
> On Thu,  4 Aug 2016 18:24:46 +0200, Romain Naour wrote:
> 
>> +diff --git a/src/core/dasd.cc b/src/core/dasd.cc
>> +index 626b8a8..18d19c3 100644
>> +--- a/src/core/dasd.cc
>> ++++ b/src/core/dasd.cc
>> +@@ -4,6 +4,7 @@
>> + #include <glob.h>
>> + #include <string.h>
>> + #include <fcntl.h>
>> ++#include <libgen.h>
> 
> Is this related?

if we want to use POSIX compliant version of basename() then we need to
include this yes.

> 
>> + #include <unistd.h>
>> + #include <inttypes.h>
>> + #include <sys/ioctl.h>
>> +@@ -42,7 +43,7 @@ bool scan_dasd(hwNode & n)
>> +   {
>> +     for(dev_num=0;dev_num<devices.gl_pathc;dev_num++)
>> +     {
>> +-      dev_name = basename(devices.gl_pathv[dev_num]);
>> ++      dev_name = basename(const_cast<char *>(devices.gl_pathv[dev_num]));
> 
> I'm not super familiar with C++ stuff, but why is this problem musl
> specific? The basename() function is "char *basename(char *)"
> regardless of the C library being used. What makes it error out with
> musl and not with other C libraries?

glibc and uclibc implement the GNU version of basename() API which does
not modify the argument ( const char*) and when including string.h and
not libgen.h we are saying we want gnu version. musl does not implement
the gnu extention, it only has POSIX version. Hence you see the issue
just on musl.

So the patch makes it compile for all libcs. however, it should be
tested on glibc/uclibc since there were bugs in the posix implementation
of basename()  see

http://man7.org/linux/man-pages/man3/basename.3.html

if it cant be tested then make the posix version use only for musl
which I would not recommend, but its an option

> 
> Thanks,
> 
> Thomas
> 

[Buildroot] [PATCH] package/lshw: fix musl build

[Yann E. MORIN]

Romain, Khem, All,

On 2016-08-04 21:25 -0700, Khem Raj spake thusly:
> On 8/4/16 9:37 AM, Thomas Petazzoni wrote:
> > Hello,
> > 
> > On Thu,  4 Aug 2016 18:24:46 +0200, Romain Naour wrote:
> > 
> >> +diff --git a/src/core/dasd.cc b/src/core/dasd.cc
> >> +index 626b8a8..18d19c3 100644
> >> +--- a/src/core/dasd.cc
> >> ++++ b/src/core/dasd.cc
> >> +@@ -4,6 +4,7 @@
> >> + #include <glob.h>
> >> + #include <string.h>
> >> + #include <fcntl.h>
> >> ++#include <libgen.h>
> > 
> > Is this related?
> 
> if we want to use POSIX compliant version of basename() then we need to
> include this yes.

Confirmed. ;-)

> >> + #include <unistd.h>
> >> + #include <inttypes.h>
> >> + #include <sys/ioctl.h>
> >> +@@ -42,7 +43,7 @@ bool scan_dasd(hwNode & n)
> >> +   {
> >> +     for(dev_num=0;dev_num<devices.gl_pathc;dev_num++)
> >> +     {
> >> +-      dev_name = basename(devices.gl_pathv[dev_num]);
> >> ++      dev_name = basename(const_cast<char *>(devices.gl_pathv[dev_num]));
> > 
> > I'm not super familiar with C++ stuff, but why is this problem musl
> > specific? The basename() function is "char *basename(char *)"
> > regardless of the C library being used. What makes it error out with
> > musl and not with other C libraries?
> 
> glibc and uclibc implement the GNU version of basename() API which does
> not modify the argument ( const char*) and when including string.h and
> not libgen.h we are saying we want gnu version. musl does not implement
> the gnu extention, it only has POSIX version. Hence you see the issue
> just on musl.
> 
> So the patch makes it compile for all libcs. however, it should be
> tested on glibc/uclibc since there were bugs in the posix implementation
> of basename()  see
> 
> http://man7.org/linux/man-pages/man3/basename.3.html
> 
> if it cant be tested then make the posix version use only for musl
> which I would not recommend, but its an option

Note however that upstream already did a similar change in an other
file (src/core/pci.cc, by the end of the commit):
    https://github.com/lyonel/lshw/commit/cd690bff1516b40fecd5ec4a7f6619e5bffc3cf0

Romain did open a pull request, but it has not yet been merged:
    https://github.com/lyonel/lshw/pull/17

However, I believe casting is utterly wrong: c_str() returns a
const char* and C++98 states that that string must *not* be modified,
but the POSIX basename() *does* modify its argument.

So, I really believe that the correct fix is completely different
(abstracting away basename, maybe?).

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH] package/lshw: fix musl build

[Khem Raj]

> On Aug 12, 2016, at 3:41 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> 
> Romain, Khem, All,
> 
> On 2016-08-04 21:25 -0700, Khem Raj spake thusly:
>> On 8/4/16 9:37 AM, Thomas Petazzoni wrote:
>>> Hello,
>>> 
>>> On Thu,  4 Aug 2016 18:24:46 +0200, Romain Naour wrote:
>>> 
>>>> +diff --git a/src/core/dasd.cc b/src/core/dasd.cc
>>>> +index 626b8a8..18d19c3 100644
>>>> +--- a/src/core/dasd.cc
>>>> ++++ b/src/core/dasd.cc
>>>> +@@ -4,6 +4,7 @@
>>>> + #include <glob.h>
>>>> + #include <string.h>
>>>> + #include <fcntl.h>
>>>> ++#include <libgen.h>
>>> 
>>> Is this related?
>> 
>> if we want to use POSIX compliant version of basename() then we need to
>> include this yes.
> 
> Confirmed. ;-)
> 
>>>> + #include <unistd.h>
>>>> + #include <inttypes.h>
>>>> + #include <sys/ioctl.h>
>>>> +@@ -42,7 +43,7 @@ bool scan_dasd(hwNode & n)
>>>> +   {
>>>> +     for(dev_num=0;dev_num<devices.gl_pathc;dev_num++)
>>>> +     {
>>>> +-      dev_name = basename(devices.gl_pathv[dev_num]);
>>>> ++      dev_name = basename(const_cast<char *>(devices.gl_pathv[dev_num]));
>>> 
>>> I'm not super familiar with C++ stuff, but why is this problem musl
>>> specific? The basename() function is "char *basename(char *)"
>>> regardless of the C library being used. What makes it error out with
>>> musl and not with other C libraries?
>> 
>> glibc and uclibc implement the GNU version of basename() API which does
>> not modify the argument ( const char*) and when including string.h and
>> not libgen.h we are saying we want gnu version. musl does not implement
>> the gnu extention, it only has POSIX version. Hence you see the issue
>> just on musl.
>> 
>> So the patch makes it compile for all libcs. however, it should be
>> tested on glibc/uclibc since there were bugs in the posix implementation
>> of basename()  see
>> 
>> http://man7.org/linux/man-pages/man3/basename.3.html
>> 
>> if it cant be tested then make the posix version use only for musl
>> which I would not recommend, but its an option
> 
> Note however that upstream already did a similar change in an other
> file (src/core/pci.cc, by the end of the commit):
>    https://github.com/lyonel/lshw/commit/cd690bff1516b40fecd5ec4a7f6619e5bffc3cf0
> 

Thats fine, if the use of the variable is limited in scope it may
not have side effects of being overwritten

> Romain did open a pull request, but it has not yet been merged:
>    https://github.com/lyonel/lshw/pull/17
> 
> However, I believe casting is utterly wrong: c_str() returns a
> const char* and C++98 states that that string must *not* be modified,
> but the POSIX basename() *does* modify its argument.
> 
> So, I really believe that the correct fix is completely different
> (abstracting away basename, maybe?).

If there are side effects maybe, otherwise casting is ok.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 204 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20160812/ce4f4368/attachment.asc>