Thread: Re: [HACKERS] compile warning in CVS HEAD
Patch attached. Also adds a malloc() check that Neil wanted.
cheers
andrew
Tom Lane wrote:
>Andrew Dunstan <andrew@dunslane.net> writes:
>
>
>>*sigh*
>>
>>
>
>
>
>>my local (linux) man for gettimeofday says this:
>>
>>
>
>
>
>> struct timeval {
>> time_t tv_sec; /* seconds */
>> suseconds_t tv_usec; /* microseconds */
>> };
>>
>>
>
>Yeah, but mine (HPUX) says that tv_sec is "unsigned long". I suspect
>that on Darwin the types disagree as to signedness.
>
>
>
>>We could do what you say, or could we just cast it?
>>
>>
>
>If they really were different types (as in different widths) then
>casting the pointer would be a highly Wrong Thing. I think copying
>to a local is safer, even if it does waste a cycle or two.
>
> regards, tom lane
>
>---------------------------(end of broadcast)---------------------------
>TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org
>
>
>
Index: src/backend/utils/error/elog.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/backend/utils/error/elog.c,v
retrieving revision 1.128
diff -c -r1.128 elog.c
*** src/backend/utils/error/elog.c 15 Mar 2004 15:56:23 -0000 1.128
--- src/backend/utils/error/elog.c 18 Mar 2004 22:55:44 -0000
***************
*** 1035,1041 ****
--- 1035,1045 ----
int result_len = 2*NAMEDATALEN + format_len +120 ;
if (result == NULL)
+ {
result = malloc(result_len);
+ if (result == NULL)
+ return "";
+ }
result[0] = '\0';
if (format_len > 0)
***************
*** 1119,1126 ****
localtime(&stamp_time));
break;
case 's':
j += strftime(result+j, result_len-j, "%Y-%m-%d %H:%M:%S",
! localtime(&(MyProcPort->session_start.tv_sec)));
break;
case 'i':
j += snprintf(result+j,result_len-j,"%s",
--- 1123,1131 ----
localtime(&stamp_time));
break;
case 's':
+ stamp_time = (time_t)(MyProcPort->session_start.tv_sec);
j += strftime(result+j, result_len-j, "%Y-%m-%d %H:%M:%S",
! localtime(&stamp_time));
break;
case 'i':
j += snprintf(result+j,result_len-j,"%s",
Andrew Dunstan <andrew@dunslane.net> writes:
> Patch attached. Also adds a malloc() check that Neil wanted.
Er, what on God's green earth is elog.c doing malloc'ing stuff at all?
This should be a palloc in the ErrorContext; that's what it's for.
As is, this code is guaranteed to fail under out-of-memory conditions.
regards, tom lane
Tom Lane wrote: >Andrew Dunstan <andrew@dunslane.net> writes: > > >>Patch attached. Also adds a malloc() check that Neil wanted. >> >> > >Er, what on God's green earth is elog.c doing malloc'ing stuff at all? >This should be a palloc in the ErrorContext; that's what it's for. >As is, this code is guaranteed to fail under out-of-memory conditions. > > > Well, it's a one-off allocation into a static variable, if that makes any difference. If not, I accept the rebuke :-) I first published the ancestor of this code many months ago, and nobody has objected until now. But there is always room for improvement I guess, including in my own knowledge. I'll be happy to change it if need be. cheers andrew
Where are we on this patch? There was concern about the malloc() but
that can be fixed.
---------------------------------------------------------------------------
Andrew Dunstan wrote:
>
> Patch attached. Also adds a malloc() check that Neil wanted.
>
> cheers
>
> andrew
>
> Tom Lane wrote:
>
> >Andrew Dunstan <andrew@dunslane.net> writes:
> >
> >
> >>*sigh*
> >>
> >>
> >
> >
> >
> >>my local (linux) man for gettimeofday says this:
> >>
> >>
> >
> >
> >
> >> struct timeval {
> >> time_t tv_sec; /* seconds */
> >> suseconds_t tv_usec; /* microseconds */
> >> };
> >>
> >>
> >
> >Yeah, but mine (HPUX) says that tv_sec is "unsigned long". I suspect
> >that on Darwin the types disagree as to signedness.
> >
> >
> >
> >>We could do what you say, or could we just cast it?
> >>
> >>
> >
> >If they really were different types (as in different widths) then
> >casting the pointer would be a highly Wrong Thing. I think copying
> >to a local is safer, even if it does waste a cycle or two.
> >
> > regards, tom lane
> >
> >---------------------------(end of broadcast)---------------------------
> >TIP 6: Have you searched our list archives?
> >
> > http://archives.postgresql.org
> >
> >
> >
> Index: src/backend/utils/error/elog.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/backend/utils/error/elog.c,v
> retrieving revision 1.128
> diff -c -r1.128 elog.c
> *** src/backend/utils/error/elog.c 15 Mar 2004 15:56:23 -0000 1.128
> --- src/backend/utils/error/elog.c 18 Mar 2004 22:55:44 -0000
> ***************
> *** 1035,1041 ****
> --- 1035,1045 ----
> int result_len = 2*NAMEDATALEN + format_len +120 ;
>
> if (result == NULL)
> + {
> result = malloc(result_len);
> + if (result == NULL)
> + return "";
> + }
> result[0] = '\0';
>
> if (format_len > 0)
> ***************
> *** 1119,1126 ****
> localtime(&stamp_time));
> break;
> case 's':
> j += strftime(result+j, result_len-j, "%Y-%m-%d %H:%M:%S",
> ! localtime(&(MyProcPort->session_start.tv_sec)));
> break;
> case 'i':
> j += snprintf(result+j,result_len-j,"%s",
> --- 1123,1131 ----
> localtime(&stamp_time));
> break;
> case 's':
> + stamp_time = (time_t)(MyProcPort->session_start.tv_sec);
> j += strftime(result+j, result_len-j, "%Y-%m-%d %H:%M:%S",
> ! localtime(&stamp_time));
> break;
> case 'i':
> j += snprintf(result+j,result_len-j,"%s",
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
OK, I talked to Andrew via IM and he says it is fixed in CVS.
---------------------------------------------------------------------------
Andrew Dunstan wrote:
>
> Patch attached. Also adds a malloc() check that Neil wanted.
>
> cheers
>
> andrew
>
> Tom Lane wrote:
>
> >Andrew Dunstan <andrew@dunslane.net> writes:
> >
> >
> >>*sigh*
> >>
> >>
> >
> >
> >
> >>my local (linux) man for gettimeofday says this:
> >>
> >>
> >
> >
> >
> >> struct timeval {
> >> time_t tv_sec; /* seconds */
> >> suseconds_t tv_usec; /* microseconds */
> >> };
> >>
> >>
> >
> >Yeah, but mine (HPUX) says that tv_sec is "unsigned long". I suspect
> >that on Darwin the types disagree as to signedness.
> >
> >
> >
> >>We could do what you say, or could we just cast it?
> >>
> >>
> >
> >If they really were different types (as in different widths) then
> >casting the pointer would be a highly Wrong Thing. I think copying
> >to a local is safer, even if it does waste a cycle or two.
> >
> > regards, tom lane
> >
> >---------------------------(end of broadcast)---------------------------
> >TIP 6: Have you searched our list archives?
> >
> > http://archives.postgresql.org
> >
> >
> >
> Index: src/backend/utils/error/elog.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/backend/utils/error/elog.c,v
> retrieving revision 1.128
> diff -c -r1.128 elog.c
> *** src/backend/utils/error/elog.c 15 Mar 2004 15:56:23 -0000 1.128
> --- src/backend/utils/error/elog.c 18 Mar 2004 22:55:44 -0000
> ***************
> *** 1035,1041 ****
> --- 1035,1045 ----
> int result_len = 2*NAMEDATALEN + format_len +120 ;
>
> if (result == NULL)
> + {
> result = malloc(result_len);
> + if (result == NULL)
> + return "";
> + }
> result[0] = '\0';
>
> if (format_len > 0)
> ***************
> *** 1119,1126 ****
> localtime(&stamp_time));
> break;
> case 's':
> j += strftime(result+j, result_len-j, "%Y-%m-%d %H:%M:%S",
> ! localtime(&(MyProcPort->session_start.tv_sec)));
> break;
> case 'i':
> j += snprintf(result+j,result_len-j,"%s",
> --- 1123,1131 ----
> localtime(&stamp_time));
> break;
> case 's':
> + stamp_time = (time_t)(MyProcPort->session_start.tv_sec);
> j += strftime(result+j, result_len-j, "%Y-%m-%d %H:%M:%S",
> ! localtime(&stamp_time));
> break;
> case 'i':
> j += snprintf(result+j,result_len-j,"%s",
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073