| Message ID | 1470327886-29273-1-git-send-email-romain.naour@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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 >
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
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 >
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 >
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.
> 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.
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 +
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