Thread: 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@bigfoot.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@candle.pha.pa.us | (610)
359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square,
Pennsylvania19073
"Bruce Momjian" <pgman@candle.pha.pa.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@candle.pha.pa.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@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Bruce Momjian <pgman@candle.pha.pa.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@candle.pha.pa.us> wrote: > Gaetano Mendola wrote: > > "Bruce Momjian" <pgman@candle.pha.pa.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@sss.pgh.pa.us> wrote: > Bruce Momjian <pgman@candle.pha.pa.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@bigfoot.com> writes:
> "Tom Lane" <tgl@sss.pgh.pa.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@sss.pgh.pa.us> wrote: > "Gaetano Mendola" <mendola@bigfoot.com> writes: > > "Tom Lane" <tgl@sss.pgh.pa.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@candle.pha.pa.us> wrote: > Gaetano Mendola wrote: > > "Bruce Momjian" <pgman@candle.pha.pa.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@candle.pha.pa.us> wrote: > > Gaetano Mendola wrote: > > > "Bruce Momjian" <pgman@candle.pha.pa.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@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
"Bruce Momjian" <pgman@candle.pha.pa.us> wrote: > Mendola Gaetano wrote: > > "Bruce Momjian" <pgman@candle.pha.pa.us> wrote: > > > Gaetano Mendola wrote: > > > > "Bruce Momjian" <pgman@candle.pha.pa.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