[Home]

Summary:ASTERISK-06913: [patch] always assume pid_t is long
Reporter:Andrey S Pankov (casper)Labels:
Date Opened:2006-05-07 09:03:00Date Closed:2006-05-08 07:36:37
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) GETPID.diff
( 1) Getpid-v2.diff
Description:Inspired by bweschke's super-fix for ASTERISK-6912 :(

For those interested: pid_t should be always defined now due to AC_TYPE_PID_T.

Why I assume pid_t to be long int? For safety and portability. Anyway on
systems where getpid() returns long there will be compiler warnings and
the patch solves this issue as well. And we use it as long already in the code.
The patch makes things consistent, doesn't it?

****** ADDITIONAL INFORMATION ******

I fully understand this may be considered as "architectural" change, but
it is not, even if it requires the change to options.h :)

And please... don't be in a hurry to commit workarounds even without
reading what is in 3-5 lines above in the code. :( Thanks!
Comments:By: BJ Weschke (bweschke) 2006-05-07 13:13:48

casper: I'm all for architectural changes if they will ultimately fix would could become a bug. That was the case with some other bugs we closed out earlier this week.

Given that we have autoconf/automake now in /trunk, would it make sense to check for the existence of AC_TYPE_PID_T and, if so, use pid_t instead of the current long?

Thanks for your cynical comments too with regard to my earlier bug marshall activities. Based on that, it makes me believe I'm doing enough to catch your attention and criticism. :) cheers!
By: Andrey S Pankov (casper) 2006-05-07 13:20:39

bweschke: could you look at the patch first. It's 100% autoconf compliant.
And it fixes compiler warnings on systems where pid_t is long.

> would it make sense to check for the existence of AC_TYPE_PID_T and, if so, use
> pid_t instead of the current long

It's already checked and I do use pid_t in the patch instead of int/long mix
present in the current code.
By: Andrey S Pankov (casper) 2006-05-07 13:24:13

And that's why I asked to wait with fixing ASTERISK-6912. Here is a chunk fixing
your fix for trunk:
if (ast_opt_always_fork || !ast_opt_no_fork) {
daemon(0, 0);
+ ast_mainpid = getpid();
/* Blindly re-write pid file since we are forking */
unlink(ast_config_AST_PID);
f = fopen(ast_config_AST_PID, "w");
if (f) {
- fprintf(f, "%d\n", (int)getpid());
+ fprintf(f, "%ld\n", (long)ast_mainpid);
fclose(f);
} else
ast_log(LOG_WARNING, "Unable to open pid file '%s': %s\n", ast_config_AST_PID, strerror(errno));
- ast_mainpid = getpid();
}
By: Denis Smirnov (mithraen) 2006-05-07 14:50:23

After fixing cast in sscanf in would be very nice fix.
By: Andrey S Pankov (casper) 2006-05-07 14:57:37

I'll update the patch since the proposed code for pbx_spool is buggy
(thanks Mithraen for pointing it out)
By: BJ Weschke (bweschke) 2006-05-07 14:59:48

casper: I did look at your patch. It doesn't make any adjustments to the autoconf infrastructure, so you're assuming that pid_t is supported and portable in all current supported platforms. Is it? I really don't know. That's why I made the suggestion to make use of what is there now in /trunk.
Really, I'm here to work with you, not against you, and I do very much appreciate the contributions. You are a smart fellow and I think our goal is a common one, a better open source telephony platform. Let's work together instead of against each other.  Asterisk is much better with you contributing than without you contributing.
By: Andrey S Pankov (casper) 2006-05-07 15:07:12

bweschke: please open configure.ac at line 646, AC_TYPE_PID_T is there.
Then, please open autoconfig.h.in at line 538.
Let me explain: if <sys/types.h> does not define pid_t TSD, then we'll have
pid_t defined to int. AC_TYPE_PID_T is equivalent to AC_CHECK_TYPE(pid_t, int).
That way pid_t is _always_ defined on all platforms, even not yet supported :)
By: Andrey S Pankov (casper) 2006-05-07 15:11:31

And please do not take it so personnaly... Your fix was a bad one,
there is no need to call getpid() twice there. I do not like
workarounds like this. Nor I don't understand how you fix to app_meetme
can solve the problem, there are lots of similar places there. And I assume
there may be memory leaks if you do break earlier... just thoughts... ;)


By: Andrey S Pankov (casper) 2006-05-07 15:16:18

Fixed version uploaded, thanks to Mithraen for a suggestion! ;)
By: BJ Weschke (bweschke) 2006-05-08 07:36:36

casper. You are absolutely right. Thanks for pointing out the autoconfig.h to me. Your latest patch has been committed to /trunk.
I think as you can see from my comments, I'm absolutely not taking anything personally. Thank you for your continuted contributions.