You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Küng <to...@gmail.com> on 2008/09/10 17:19:32 UTC
[PATCH] initialize struct members
Hi,
Before I commit this, maybe someone likes to first review it:
this patch initializes all struct members in the functions
svn_wc_conflict_description_create_text() and
svn_wc_conflict_description_create_prop().
Found this due to crashes in TSVN - it tried to access the merged_file
member which had a non-NULL value but pointed to bogus memory.
If nobody objects, I'll commit this in about an hour (shouldn't be much
to object to IMHO).
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [PATCH] initialize struct members
Posted by Greg Stein <gs...@gmail.com>.
Yeah, that's probably picking nits, and about a relatively minor
change. ie diminishing returns to change
On Sep 10, 2008, at 16:47, Stefan Sperling <st...@elego.de> wrote:
> On Wed, Sep 10, 2008 at 09:11:43PM +0200, Stefan Küng wrote:
>> Stefan Sperling wrote:
>>> On Wed, Sep 10, 2008 at 08:01:12PM +0200, Lieven Govaerts wrote:
>>>> Stefan Küng wrote:
>>>>> Hi,
>>>>>
>>>>> Before I commit this, maybe someone likes to first review it:
>>>>> this patch initializes all struct members in the functions
>>>>> svn_wc_conflict_description_create_text() and
>>>>> svn_wc_conflict_description_create_prop().
>>>>> Found this due to crashes in TSVN - it tried to access the
>>>>> merged_file
>>>>> member which had a non-NULL value but pointed to bogus memory.
>>>>>
>>>>> If nobody objects, I'll commit this in about an hour (shouldn't
>>>>> be much
>>>>> to object to IMHO).
>>>>>
>>>> Using apr_pcalloc has the same effect, but slightly easier for
>>>> the eyes.
>>>
>>> Oops, good point, actually.
>>>
>>> Stefan (you, not me :), do you want to make a follow-up commit that
>>> uses apr_pcalloc() instead?
>>
>> See r33019.
>
> Thanks!
>
>> Hope the commit message is ok.
>
> I took a look (because you asked).
>
> For reference, the log message:
>
> [[[
> Follow-up to r33017:
>
> Use apr_pcalloc to initialize svn_wc_conflict_description_t factory
> functions.
>
> * subversion/libsvn_wc/util.c
> (svn_wc_conflict_description_create_text),
> (svn_wc_conflict_description_create_prop): Instead of setting not
> relevant members to NULL, use apr_pcalloc instead of apr_palloc
> to get
> the memory for the structure.
> ]]]
>
> The first sentence looks wrong. It's not the functions themselves
> that are being initialised. Rather, the objects the functions are
> allocating are initialised.
>
> Also, the nested "instead of" looks a bit confusing to me (but
> probably just me).
>
> FWIW, I'd suggest something like:
>
> [[[
> Follow-up to r33017:
>
> * subversion/libsvn_wc/util.c
> (svn_wc_conflict_description_create_text),
> (svn_wc_conflict_description_create_prop): Use apr_pcalloc instead
> of apr_palloc to allocate memory. This way, we don't need to set
> any members to NULL manually.
> ]]]
>
> That said, log messages are a matter of taste and writing style,
> so you'd probably get entirely different suggestions from everyone
> else :)
>
> Thanks,
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] initialize struct members
Posted by Stefan Küng <to...@gmail.com>.
Stefan Sperling wrote:
> On Wed, Sep 10, 2008 at 09:11:43PM +0200, Stefan Küng wrote:
>> Stefan Sperling wrote:
>>> On Wed, Sep 10, 2008 at 08:01:12PM +0200, Lieven Govaerts wrote:
>>>> Stefan Küng wrote:
>>>>> Hi,
>>>>>
>>>>> Before I commit this, maybe someone likes to first review it:
>>>>> this patch initializes all struct members in the functions
>>>>> svn_wc_conflict_description_create_text() and
>>>>> svn_wc_conflict_description_create_prop().
>>>>> Found this due to crashes in TSVN - it tried to access the merged_file
>>>>> member which had a non-NULL value but pointed to bogus memory.
>>>>>
>>>>> If nobody objects, I'll commit this in about an hour (shouldn't be much
>>>>> to object to IMHO).
>>>>>
>>>> Using apr_pcalloc has the same effect, but slightly easier for the eyes.
>>> Oops, good point, actually.
>>>
>>> Stefan (you, not me :), do you want to make a follow-up commit that
>>> uses apr_pcalloc() instead?
>> See r33019.
>
> Thanks!
>
>> Hope the commit message is ok.
>
> I took a look (because you asked).
>
> For reference, the log message:
>
> [[[
> Follow-up to r33017:
>
> Use apr_pcalloc to initialize svn_wc_conflict_description_t factory
> functions.
>
> * subversion/libsvn_wc/util.c
> (svn_wc_conflict_description_create_text),
> (svn_wc_conflict_description_create_prop): Instead of setting not
> relevant members to NULL, use apr_pcalloc instead of apr_palloc to get
> the memory for the structure.
> ]]]
>
> The first sentence looks wrong. It's not the functions themselves
> that are being initialised. Rather, the objects the functions are
> allocating are initialised.
>
> Also, the nested "instead of" looks a bit confusing to me (but
> probably just me).
>
> FWIW, I'd suggest something like:
>
> [[[
> Follow-up to r33017:
>
> * subversion/libsvn_wc/util.c
> (svn_wc_conflict_description_create_text),
> (svn_wc_conflict_description_create_prop): Use apr_pcalloc instead
> of apr_palloc to allocate memory. This way, we don't need to set
> any members to NULL manually.
> ]]]
>
> That said, log messages are a matter of taste and writing style,
> so you'd probably get entirely different suggestions from everyone
> else :)
Thanks. Your log message looks much better.
I've changed it to your version.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [PATCH] initialize struct members
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Sep 10, 2008 at 09:11:43PM +0200, Stefan Küng wrote:
> Stefan Sperling wrote:
> > On Wed, Sep 10, 2008 at 08:01:12PM +0200, Lieven Govaerts wrote:
> >> Stefan Küng wrote:
> >>> Hi,
> >>>
> >>> Before I commit this, maybe someone likes to first review it:
> >>> this patch initializes all struct members in the functions
> >>> svn_wc_conflict_description_create_text() and
> >>> svn_wc_conflict_description_create_prop().
> >>> Found this due to crashes in TSVN - it tried to access the merged_file
> >>> member which had a non-NULL value but pointed to bogus memory.
> >>>
> >>> If nobody objects, I'll commit this in about an hour (shouldn't be much
> >>> to object to IMHO).
> >>>
> >> Using apr_pcalloc has the same effect, but slightly easier for the eyes.
> >
> > Oops, good point, actually.
> >
> > Stefan (you, not me :), do you want to make a follow-up commit that
> > uses apr_pcalloc() instead?
>
> See r33019.
Thanks!
> Hope the commit message is ok.
I took a look (because you asked).
For reference, the log message:
[[[
Follow-up to r33017:
Use apr_pcalloc to initialize svn_wc_conflict_description_t factory
functions.
* subversion/libsvn_wc/util.c
(svn_wc_conflict_description_create_text),
(svn_wc_conflict_description_create_prop): Instead of setting not
relevant members to NULL, use apr_pcalloc instead of apr_palloc to get
the memory for the structure.
]]]
The first sentence looks wrong. It's not the functions themselves
that are being initialised. Rather, the objects the functions are
allocating are initialised.
Also, the nested "instead of" looks a bit confusing to me (but
probably just me).
FWIW, I'd suggest something like:
[[[
Follow-up to r33017:
* subversion/libsvn_wc/util.c
(svn_wc_conflict_description_create_text),
(svn_wc_conflict_description_create_prop): Use apr_pcalloc instead
of apr_palloc to allocate memory. This way, we don't need to set
any members to NULL manually.
]]]
That said, log messages are a matter of taste and writing style,
so you'd probably get entirely different suggestions from everyone
else :)
Thanks,
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] initialize struct members
Posted by Stefan Küng <to...@gmail.com>.
Stefan Sperling wrote:
> On Wed, Sep 10, 2008 at 08:01:12PM +0200, Lieven Govaerts wrote:
>> Stefan Küng wrote:
>>> Hi,
>>>
>>> Before I commit this, maybe someone likes to first review it:
>>> this patch initializes all struct members in the functions
>>> svn_wc_conflict_description_create_text() and
>>> svn_wc_conflict_description_create_prop().
>>> Found this due to crashes in TSVN - it tried to access the merged_file
>>> member which had a non-NULL value but pointed to bogus memory.
>>>
>>> If nobody objects, I'll commit this in about an hour (shouldn't be much
>>> to object to IMHO).
>>>
>> Using apr_pcalloc has the same effect, but slightly easier for the eyes.
>
> Oops, good point, actually.
>
> Stefan (you, not me :), do you want to make a follow-up commit that
> uses apr_pcalloc() instead?
See r33019.
Hope the commit message is ok.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [PATCH] initialize struct members
Posted by Stefan Küng <to...@gmail.com>.
Stefan Sperling wrote:
> On Wed, Sep 10, 2008 at 08:01:12PM +0200, Lieven Govaerts wrote:
>> Stefan Küng wrote:
>>> Hi,
>>>
>>> Before I commit this, maybe someone likes to first review it:
>>> this patch initializes all struct members in the functions
>>> svn_wc_conflict_description_create_text() and
>>> svn_wc_conflict_description_create_prop().
>>> Found this due to crashes in TSVN - it tried to access the merged_file
>>> member which had a non-NULL value but pointed to bogus memory.
>>>
>>> If nobody objects, I'll commit this in about an hour (shouldn't be much
>>> to object to IMHO).
>>>
>> Using apr_pcalloc has the same effect, but slightly easier for the eyes.
>
> Oops, good point, actually.
>
> Stefan (you, not me :), do you want to make a follow-up commit that
> uses apr_pcalloc() instead?
Will do. I'm currently running a rebuild and do some tests before I
commit the change.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [PATCH] initialize struct members
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Sep 10, 2008 at 08:01:12PM +0200, Lieven Govaerts wrote:
> Stefan Küng wrote:
> > Hi,
> >
> > Before I commit this, maybe someone likes to first review it:
> > this patch initializes all struct members in the functions
> > svn_wc_conflict_description_create_text() and
> > svn_wc_conflict_description_create_prop().
> > Found this due to crashes in TSVN - it tried to access the merged_file
> > member which had a non-NULL value but pointed to bogus memory.
> >
> > If nobody objects, I'll commit this in about an hour (shouldn't be much
> > to object to IMHO).
> >
> Using apr_pcalloc has the same effect, but slightly easier for the eyes.
Oops, good point, actually.
Stefan (you, not me :), do you want to make a follow-up commit that
uses apr_pcalloc() instead?
Thanks,
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] initialize struct members
Posted by Lieven Govaerts <sv...@mobsol.be>.
Stefan Küng wrote:
> Hi,
>
> Before I commit this, maybe someone likes to first review it:
> this patch initializes all struct members in the functions
> svn_wc_conflict_description_create_text() and
> svn_wc_conflict_description_create_prop().
> Found this due to crashes in TSVN - it tried to access the merged_file
> member which had a non-NULL value but pointed to bogus memory.
>
> If nobody objects, I'll commit this in about an hour (shouldn't be much
> to object to IMHO).
>
Using apr_pcalloc has the same effect, but slightly easier for the eyes.
Lieven
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] initialize struct members
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Sep 10, 2008 at 07:50:41PM +0200, Stefan Küng wrote:
> Stefan Sperling wrote:
> > On Wed, Sep 10, 2008 at 07:19:32PM +0200, Stefan Küng wrote:
> >> Hi,
> >>
> >> Before I commit this, maybe someone likes to first review it:
> >> this patch initializes all struct members in the functions
> >> svn_wc_conflict_description_create_text() and
> >> svn_wc_conflict_description_create_prop().
> >> Found this due to crashes in TSVN - it tried to access the merged_file
> >> member which had a non-NULL value but pointed to bogus memory.
> >
> > +1, referencing NULL is bad, but bogus memory is even worse.
>
> Committed in r33017.
Thanks. I've tweaked the log message a bit.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] initialize struct members
Posted by Stefan Küng <to...@gmail.com>.
Stefan Sperling wrote:
> On Wed, Sep 10, 2008 at 07:19:32PM +0200, Stefan Küng wrote:
>> Hi,
>>
>> Before I commit this, maybe someone likes to first review it:
>> this patch initializes all struct members in the functions
>> svn_wc_conflict_description_create_text() and
>> svn_wc_conflict_description_create_prop().
>> Found this due to crashes in TSVN - it tried to access the merged_file
>> member which had a non-NULL value but pointed to bogus memory.
>
> +1, referencing NULL is bad, but bogus memory is even worse.
Committed in r33017.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [PATCH] initialize struct members
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Sep 10, 2008 at 07:19:32PM +0200, Stefan Küng wrote:
> Hi,
>
> Before I commit this, maybe someone likes to first review it:
> this patch initializes all struct members in the functions
> svn_wc_conflict_description_create_text() and
> svn_wc_conflict_description_create_prop().
> Found this due to crashes in TSVN - it tried to access the merged_file
> member which had a non-NULL value but pointed to bogus memory.
+1, referencing NULL is bad, but bogus memory is even worse.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org