[replying to multiple]
James McCoy wrote on Fri, Sep 01, 2017 at 08:15:05 -0400:
> On Fri, Sep 01, 2017 at 03:07:16AM +0000, Daniel Shahaf wrote:
> > I'm not sure I'm getting through here.
> >
> > The note does say "Don't allocate one of these yourself" but in the
> > second sentence implies that the reason for this is that if you allocate
> > it yourself and don't initialize all pointer-to-function members,
> > Trouble will ensue. Therefore, the note could be construed as meaning
> > that it is okay to allocate one of these yourself if you take care to
> > initialize all pointer members.
>
> The fact that a user can choose not to use the recommended API doesn't
> mean that there is an ABI break. A user can already choose to allocate
> the structure themselves and not initialize everything. Adding an item
> to the struct doesn't change that.
>
I'm afraid I don't follow. If a user allocates the struct themselves
and doesn't initialize all members, then (1) they are in violation of
the API's precondition, (2) their code will (depending on the repository
data) dereference an uninitialized pointer-to-function at runtime.
That's undefined behaviour de jure and some form of crash de facto.
> I ran abi-compliance-checker[0] against the head of branches/1.9.x and
> trunk. It shows no incompatibility issues. The report is attached for
> anyone that wants to view it.
>
> [0]: http://ispras.linuxbase.org/index.php/ABI_compliance_checker
Thanks for adding this datapoint, but, how does it support your
argument? The checker's output confirms what we already knew without
it: that adding a member to a struct type can result in breakage when
code compiled against 1.9 is run against 1.10 without rebuilding:
> [Checker's output:]
> Change Effect
> 1) This field will not be initialized by old clients.
> 1 Field apply_textdelta_stream has been added to this type. 2) Size of the inclusive type has been changed.
> NOTE: this field should be accessed only from the new library functions, otherwise it may
> result in crash or incorrect behavior of applications.
>
> 2 Size of this type has been changed from 128 bytes to 136 bytes. The fields or parameters of such data type may be incorrectly initialized or accessed by old
> client applications.
r1803143 accesses the 'apply_textdelta_stream' member without checking
whether the svn_delta_editor_t struct it was handed had been initialized
by code compiled against 1.9 or against 1.10.
Evgeny Kotkov wrote on Fri, Sep 01, 2017 at 17:13:10 +0300:
> Daniel Shahaf <d.s_at_daniel.shahaf.name> writes:
>
> > I'm not sure I'm getting through here.
> >
> > The note does say "Don't allocate one of these yourself" but in the
> > second sentence implies that the reason for this is that if you allocate
> > it yourself and don't initialize all pointer-to-function members,
> > Trouble will ensue. Therefore, the note could be construed as meaning
> > that it is okay to allocate one of these yourself if you take care to
> > initialize all pointer members.
> >
> > In other words: the comment could have been interpreted as a piece
> > of advice --- "it is more robust to use _default_editor() than to allocate
> > with malloc" --- as opposed to a blanket prohibition on allocating this
> > struct type with malloc.
> >
> > (If one uses malloc and doesn't initialize all pointers, the result
> > would be that an uninitialized pointer-to-function struct member is
> > dereferenced.)
> >
> > In contrast, most of our other structs explicitly say that "Don't
> > allocate yourself because we may add new fields in the future". This
> > struct does not give that reason.
> >
> > Makes sense?
> >
> > I suppose can just add an API erratum and/or release notes entry about this.
>
> I think that the important thing about this documentation is that it
> states that:
>
> (1) The API user shouldn't try to allocate the struct himself.
>
> (2) A constructor such as svn_delta_default_editor() should *always*
> be used.
>
> To my mind, the current statement is clear and it is not possible to
> interpret it as if it's allowed to malloc() the struct and initialize it
> manually.
Then we shall have to agree to disagree about the @note's
interpretation.
> Also, I am not against the idea of tweaking the note by saying something
> like "...because we may add new fields in the future", but I don't think
> that it is required either. (In other words, I am +0 to that.)
Done in r1807028.
> My opinion here is that neither the API erratum nor including this in the
> release notes are required, and doing so would just unnecessarily restate
> the information that's already available.
>
If I found the docstring unambiguous, I would agree that errata and
release notes entries were redundant and superfluous.
However, I find the docstring ambiguous and you do not. I think in this
situation, a release notes entry is warranted; I would think so even if
I were the one who found the docstring unambiguous and you were the one
who found it ambiguous. (In other words: I agree with Stefan.)
I think the costs here are asymmetric: the cost of adding a caution note
when it is unneeded is small, the cost of not adding a caution note when
it is needed is large.
Cheers,
Daniel
Received on 2017-09-02 10:18:17 CEST