Re: SetVariable

Hi all,
I found this code on the file variables.c and
in the function SetVariable I read:

if (strcmp(current->name, name) == 0)
{
free(current->value);
current->value = strdup(value);
return current->value ? true : false;
}

this mean that if there is no memory left on the
sistem we loose the old value,
if this is not the indeended behaviour may be is better do:

if (strcmp(current->name, name) == 0)
{
char * tmp_value = strdup(value);

if ( !tmp_value )
{
return false;
}

free(current->value);
current->value = tmp_value;

return true;
}

Regards
Gaetano Mendola

Just a follow up,
is it better to give a patch for this kind of stuff ?

Regards
Gaetano Mendola

""Mendola Gaetano"" <mendola(at)bigfoot(dot)com> wrote:
> Hi all,
> I found this code on the file variables.c and
> in the function SetVariable I read:
>
> if (strcmp(current->name, name) == 0)
> {
> free(current->value);
> current->value = strdup(value);
> return current->value ? true : false;
> }
>
> this mean that if there is no memory left on the
> sistem we loose the old value,
> if this is not the indeended behaviour may be is better do:
>
> if (strcmp(current->name, name) == 0)
> {
> char * tmp_value = strdup(value);
>
> if ( !tmp_value )
> {
> return false;
> }
>
> free(current->value);
> current->value = tmp_value;
>
> return true;
> }
>
>
> Regards
> Gaetano Mendola
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 8: explain analyze is your friend
>

Mendola Gaetano wrote:
> Hi all,
> I found this code on the file variables.c and
> in the function SetVariable I read:
>
> if (strcmp(current->name, name) == 0)
> {
> free(current->value);
> current->value = strdup(value);
> return current->value ? true : false;
> }
>
> this mean that if there is no memory left on the
> sistem we loose the old value,
> if this is not the indeended behaviour may be is better do:

I see other strdup() calls that don't check on a return. Should we deal
with those too?

>
> if (strcmp(current->name, name) == 0)
> {
> char * tmp_value = strdup(value);
>
> if ( !tmp_value )
> {
> return false;
> }
>
> free(current->value);
> current->value = tmp_value;
>
> return true;
> }
>
>
> Regards
> Gaetano Mendola
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 8: explain analyze is your friend
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

"Bruce Momjian" <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> I see other strdup() calls that don't check on a return. Should we deal
> with those too?

Well strdup obtain the memory for the new string using a malloc
and normally is a good habit check the return value of a malloc.

Regards
Gaetano Mendola

Gaetano Mendola wrote:
> "Bruce Momjian" <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> > I see other strdup() calls that don't check on a return. Should we deal
> > with those too?
>
> Well strdup obtain the memory for the new string using a malloc
> and normally is a good habit check the return value of a malloc.

Right. My point is that we have lots of other strdup's in the code.
Should we fix those too? Seems we should be consistent.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> I see other strdup() calls that don't check on a return. Should we deal
> with those too?

strdup -> xstrdup if you're concerned.

regards, tom lane

"Bruce Momjian" <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> Gaetano Mendola wrote:
> > "Bruce Momjian" <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> > > I see other strdup() calls that don't check on a return. Should we
deal
> > > with those too?
> >
> > Well strdup obtain the memory for the new string using a malloc
> > and normally is a good habit check the return value of a malloc.
>
> Right. My point is that we have lots of other strdup's in the code.
> Should we fix those too? Seems we should be consistent.

Of course yes, consider also that inside SetVariable the check is
performed but too late, after that the old value was loose for ever.

Keep also the suggestion of Tom Late about the xstrdup.

Regards
Gaetano Mendola

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > I see other strdup() calls that don't check on a return. Should we deal
> > with those too?
>
> strdup -> xstrdup if you're concerned.

May be is a good idea avoid the future use:

#define strdup STRDUP_DEPRECATED_USE_INSTEAD_XSTRDUP

I don't like the other solution: redefine the strdup in function of xstrdup.

Regards
Gaetano Mendola

"Gaetano Mendola" <mendola(at)bigfoot(dot)com> writes:
> "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> strdup -> xstrdup if you're concerned.

> May be is a good idea avoid the future use:
> #define strdup STRDUP_DEPRECATED_USE_INSTEAD_XSTRDUP

Not a good idea --- there are places that want to check for strdup
failure and do something different than exit(1).

regards, tom lane

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Gaetano Mendola" <mendola(at)bigfoot(dot)com> writes:
> > "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> strdup -> xstrdup if you're concerned.
>
> > May be is a good idea avoid the future use:
> > #define strdup STRDUP_DEPRECATED_USE_INSTEAD_XSTRDUP
>
> Not a good idea --- there are places that want to check for strdup
> failure and do something different than exit(1).

That's true.

Regards
Gaetano Mendola

"Bruce Momjian" <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> Gaetano Mendola wrote:
> > "Bruce Momjian" <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> > > I see other strdup() calls that don't check on a return. Should we
deal
> > > with those too?
> >
> > Well strdup obtain the memory for the new string using a malloc
> > and normally is a good habit check the return value of a malloc.
>
> Right. My point is that we have lots of other strdup's in the code.
> Should we fix those too? Seems we should be consistent.

Shall I post the patch for SetVariable ?

I know that this change is not so important but I want to try
on my skin the cycle
seen somethink wrong => send patch => be applyed

Regards
Gaetano Mendola

Mendola Gaetano wrote:
> "Bruce Momjian" <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> > Gaetano Mendola wrote:
> > > "Bruce Momjian" <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> > > > I see other strdup() calls that don't check on a return. Should we
> deal
> > > > with those too?
> > >
> > > Well strdup obtain the memory for the new string using a malloc
> > > and normally is a good habit check the return value of a malloc.
> >
> > Right. My point is that we have lots of other strdup's in the code.
> > Should we fix those too? Seems we should be consistent.
>
> Shall I post the patch for SetVariable ?
>
> I know that this change is not so important but I want to try
> on my skin the cycle
> seen somethink wrong => send patch => be applyed

Sure.

It would be good if you would evaluate all the strdups, find the ones
that don't check properly, and submit a big patch of all those. The fix
can be to call xstrdup, or to check the return code.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

"Bruce Momjian" <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> Mendola Gaetano wrote:
> > "Bruce Momjian" <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> > > Gaetano Mendola wrote:
> > > > "Bruce Momjian" <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> > > > > I see other strdup() calls that don't check on a return. Should
we
> > deal
> > > > > with those too?
> > > >
> > > > Well strdup obtain the memory for the new string using a malloc
> > > > and normally is a good habit check the return value of a malloc.
> > >
> > > Right. My point is that we have lots of other strdup's in the code.
> > > Should we fix those too? Seems we should be consistent.
> >
> > Shall I post the patch for SetVariable ?
> >
> > I know that this change is not so important but I want to try
> > on my skin the cycle
> > seen somethink wrong => send patch => be applyed
>
> Sure.
>
> It would be good if you would evaluate all the strdups, find the ones
> that don't check properly, and submit a big patch of all those. The fix
> can be to call xstrdup, or to check the return code.

I will.

Regards
Gaetano Mendola