Hurd build failure

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Hurd build failure

Jean-Michel Vourgère
Hi

I'm having a look at the history of problems caused by usage of
PATH_MAX. There has been a few proposals since August 2013, but they
were based on a Debian / GNU Linux version that already contained a
patch for hurd, which was introduced back in 2009, and never made it to
the official repository.

Patches by Svante Signell only are about rrd_daemon.c and rrd_client.c.
So, I'm pretty sure they assume the other occurrences have been fixed.

Attached is the original patch by Marc Dequènes for rrd_graph.c,
rrd_graph.h and rrd_tool.c, refreshed against branch 1.5.

In Debian and derivatives, it's been applied to every version since
2009. I reviewed it and it still seems ok.
I think it's just missing a conditional free(im->graphfile) in im_free().

Before creating a pull request, I'd like you opinion about the #if usage:
On one hand, it's nice to have it, since we avoid a malloc and use the heap.
On the other hand, it makes the code more complex, and filename
allocation during graphical operation probably doesn't use a lot of
ressources compared to cairo ploting, so it doesn't seem worth the trouble.

I slightly prefer version that works everywhere, and would like to
remove the static length usage, so that the code is more simple. How
does that sound?
Shall I make a request against master or against the 1.5 branch?

I saw some questions in the list about whether hurd is broken for not
defining PATH_MAX. If I understand correctly, PATH_MAX is not part of
posix. Further more, if an OS set the file name size limit to 4k, 64k or
even much more, there will be issues using the stack. Yes it's a pain,
but in my opinion, that's the right thing to do.

--
Nirgal

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers

hurd_2009.diff (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Hurd build failure

oetiker
Administrator
Hi Jean-Michel,

Yesterday Jean-Michel Vourgère wrote:

> Hi
>
> I'm having a look at the history of problems caused by usage of
> PATH_MAX. There has been a few proposals since August 2013, but they
> were based on a Debian / GNU Linux version that already contained a
> patch for hurd, which was introduced back in 2009, and never made it to
> the official repository.
>
> Patches by Svante Signell only are about rrd_daemon.c and rrd_client.c.
> So, I'm pretty sure they assume the other occurrences have been fixed.
>
> Attached is the original patch by Marc Dequènes for rrd_graph.c,
> rrd_graph.h and rrd_tool.c, refreshed against branch 1.5.
>
> In Debian and derivatives, it's been applied to every version since
> 2009. I reviewed it and it still seems ok.
> I think it's just missing a conditional free(im->graphfile) in im_free().
>
> Before creating a pull request, I'd like you opinion about the #if usage:
> On one hand, it's nice to have it, since we avoid a malloc and use the heap.
> On the other hand, it makes the code more complex, and filename
> allocation during graphical operation probably doesn't use a lot of
> ressources compared to cairo ploting, so it doesn't seem worth the trouble.
>
> I slightly prefer version that works everywhere, and would like to
> remove the static length usage, so that the code is more simple. How
> does that sound?
> Shall I make a request against master or against the 1.5 branch?
>
> I saw some questions in the list about whether hurd is broken for not
> defining PATH_MAX. If I understand correctly, PATH_MAX is not part of
> posix. Further more, if an OS set the file name size limit to 4k, 64k or
> even much more, there will be issues using the stack. Yes it's a pain,
> but in my opinion, that's the right thing to do.
if you send a pull request (vs 1.5) I can make comments in the
request ...

the first part looks fine by me

for the second part you should ammend configure.ac to test for the
presnece of the get_current_dir_name function

AND I would like to only use it when not MAXPATH is present

cheers
tobi


>
>

--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
www.oetiker.ch [hidden email] +41 62 775 9902

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: Hurd build failure

Jean-Michel Vourgère
Tobias Oetiker wrote:

> Yesterday Jean-Michel Vourgère wrote:
>> --- rrdtool.orig/src/rrd_tool.c
>> +++ rrdtool/src/rrd_tool.c
>> @@ -598,4 +598,7 @@ int HandleInputLine(
>> +#ifdef __GLIBC__
>> +            cwd = get_current_dir_name();
>> +#else
>>              cwd = getcwd(NULL, MAXPATH);
>> +#endif
>>              if (cwd == NULL) {
>>                  printf("ERROR: getcwd %s\n", rrd_strerror(errno));
>>                  return (1);

> for the second part you should ammend configure.ac to test for the
> presence of the get_current_dir_name function
>
> AND I would like to only use it when not MAXPATH is present

I agree the ifdef __GLIBC__ was quite ugly.

Isn't this enough? :
> #ifdef MAXPATH
>   cwd = getcwd(NULL, MAXPATH);
> #else
>   cwd = get_current_dir_name();
> #endif

Or would you prefer this (after updating configure.ac) ?
> #ifdef MAXPATH
>   cwd = getcwd(NULL, MAXPATH);
> #elif define(HAVE_GET_CURRENT_DIR_NAME)
>   cwd = get_current_dir_name();
> #else
> #error "you must have either MAXPATH or get_current_dir_name()"
> #endif


Before I go on with the patches I have another base question:
src/rrd_config_bottom.h contains:

> #ifdef HAVE_SYS_PARAM_H
> # include <sys/param.h>
> #endif
> #ifndef MAXPATH
> # ifdef PATH_MAX
> #  define MAXPATH PATH_MAX
> # endif
> #endif
> #ifndef MAXPATH
> /* else try the BSD variant */
> # ifdef MAXPATHLEN
> #  define MAXPATH MAXPATHLEN
> # endif
> #endif

So I guess one should change all occurrences of PATH_MAX
(src/rrd_client.c, src/rrd_daemon.c) by MAXPATH, right?
Or am I missing something?

Also rrd_graph.h contains:
> #ifdef WIN32
> #  include <windows.h>
> #  define MAXPATH MAX_PATH
> #endif

Wouldn't it make sense to move the windows specific code in
src/rrd_config_bottom.h ?

> #ifdef HAVE_SYS_PARAM_H
> # include <sys/param.h>
> #endif
> #ifndef MAXPATH
> # ifdef PATH_MAX
> #  define MAXPATH PATH_MAX
> # endif
> #endif
> #ifndef MAXPATH
> /* else try the BSD variant */
> # ifdef MAXPATHLEN
> #  define MAXPATH MAXPATHLEN
> # endif
> #endif
> #if !defined(MAXPATH) && defined(WIN32)
> #  include <windows.h>
> #  define MAXPATH MAX_PATH
> #endif

--
Nirgal

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: Hurd build failure

oetiker
Administrator
Hi Jean-Michel,

Today Jean-Michel Vourgère wrote:

> Tobias Oetiker wrote:
> > Yesterday Jean-Michel Vourgère wrote:
> >> --- rrdtool.orig/src/rrd_tool.c
> >> +++ rrdtool/src/rrd_tool.c
> >> @@ -598,4 +598,7 @@ int HandleInputLine(
> >> +#ifdef __GLIBC__
> >> +            cwd = get_current_dir_name();
> >> +#else
> >>              cwd = getcwd(NULL, MAXPATH);
> >> +#endif
> >>              if (cwd == NULL) {
> >>                  printf("ERROR: getcwd %s\n", rrd_strerror(errno));
> >>                  return (1);
>
> > for the second part you should ammend configure.ac to test for the
> > presence of the get_current_dir_name function
> >
> > AND I would like to only use it when not MAXPATH is present
>
> I agree the ifdef __GLIBC__ was quite ugly.
>
> Isn't this enough? :
> > #ifdef MAXPATH
> >   cwd = getcwd(NULL, MAXPATH);
> > #else
> >   cwd = get_current_dir_name();
> > #endif
>
> Or would you prefer this (after updating configure.ac) ?

> > #ifdef MAXPATH
> >   cwd = getcwd(NULL, MAXPATH);
> > #elif define(HAVE_GET_CURRENT_DIR_NAME)
> >   cwd = get_current_dir_name();
> > #else
> > #error "you must have either MAXPATH or get_current_dir_name()"
> > #endif

yep ... since you would otherwhise just assume the presence of
get_current_dir_name despite it being a gnu extension.

> Before I go on with the patches I have another base question:
> src/rrd_config_bottom.h contains:
> > #ifdef HAVE_SYS_PARAM_H
> > # include <sys/param.h>
> > #endif
> > #ifndef MAXPATH
> > # ifdef PATH_MAX
> > #  define MAXPATH PATH_MAX
> > # endif
> > #endif
> > #ifndef MAXPATH
> > /* else try the BSD variant */
> > # ifdef MAXPATHLEN
> > #  define MAXPATH MAXPATHLEN
> > # endif
> > #endif
>
> So I guess one should change all occurrences of PATH_MAX
> (src/rrd_client.c, src/rrd_daemon.c) by MAXPATH, right?
> Or am I missing something?
yes that would be sensible I think ...

> Also rrd_graph.h contains:
> > #ifdef WIN32
> > #  include <windows.h>
> > #  define MAXPATH MAX_PATH
> > #endif
>
> Wouldn't it make sense to move the windows specific code in
> src/rrd_config_bottom.h ?
> > #ifdef HAVE_SYS_PARAM_H
> > # include <sys/param.h>
> > #endif
> > #ifndef MAXPATH
> > # ifdef PATH_MAX
> > #  define MAXPATH PATH_MAX
> > # endif
> > #endif
> > #ifndef MAXPATH
> > /* else try the BSD variant */
> > # ifdef MAXPATHLEN
> > #  define MAXPATH MAXPATHLEN
> > # endif
> > #endif
> > #if !defined(MAXPATH) && defined(WIN32)
> > #  include <windows.h>
> > #  define MAXPATH MAX_PATH
> > #endif
sorry I am not paying too close attention to the windows port ...
but they are not running configure, so they would have to read this
file otherwise ...

cheers
tobi


>

--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
www.oetiker.ch [hidden email] +41 62 775 9902

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: Hurd build failure

Svante Signell
On Tue, 2015-07-28 at 16:16 +0200, Tobias Oetiker wrote:
> Hi Jean-Michel,
>
> Today Jean-Michel Vourgère wrote:
...
> > Before I go on with the patches I have another base question:
> > src/rrd_config_bottom.h contains:

> > > #ifndef MAXPATH
> > > # ifdef PATH_MAX
> > > #  define MAXPATH PATH_MAX
> > > # endif
> > > #endif
> > > #ifndef MAXPATH
> > > /* else try the BSD variant */
> > > # ifdef MAXPATHLEN
> > > #  define MAXPATH MAXPATHLEN
> > > # endif
> > > #endif
> >
> > So I guess one should change all occurrences of PATH_MAX
> > (src/rrd_client.c, src/rrd_daemon.c) by MAXPATH, right?
> > Or am I missing something?

Hi, maybe I could update my patches to latest git, and we create a
working rrd-tool also for GNU/Hurd. I have checked that my patches are
clean for rrd_client.c and am almost sure they are also for
rrd_daemon.c. Any easy way to run a test case for rrd_daemon with
valgrind?

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: Hurd build failure

Jean-Michel Vourgère
Svante Signell wrote:
> Hi, maybe I could update my patches to latest git, and we create a
> working rrd-tool also for GNU/Hurd. I have checked that my patches are
> clean for rrd_client.c and am almost sure they are also for
> rrd_daemon.c. Any easy way to run a test case for rrd_daemon with
> valgrind?

I just pushed a fresh patch for:
rrd_tool.c
rrd_graph.h
rrd_graph.c
rrd_xport.c

there https://github.com/oetiker/rrdtool-1.x/pull/643
(2 commits)

So that just leave rrd_client and rrd_daemon.

Swante: Is your fork available on github? What is the address? I
couldn't find your previous version there...

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: Hurd build failure

Jean-Michel Vourgère
In reply to this post by Svante Signell
Hi

> Hi, maybe I could update my patches to latest git, and we create a
> working rrd-tool also for GNU/Hurd. I have checked that my patches are
> clean for rrd_client.c and am almost sure they are also for
> rrd_daemon.c.

rrd_tool.c, rrd_graph.h, rrd_graph.c, and rrd_xport.c are now ok in
branch 1.5.

I just made an rrd_client.c proposal at:
https://github.com/oetiker/rrdtool-1.x/pull/646

If accepted, that will only leave src/rrd_daemon.c.

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers