bug#55952: [PATCH] bindat (strz): Write null terminator after variable length string

[Eli Zaretskii]

> Date: Wed, 15 Jun 2022 14:49:58 -0400
> Cc: 55952@debbugs.gnu.org, monnier@iro.umontreal.ca
> From: Richard Hansen <rhansen@rhansen.org>
> 
> >> The existing code advances the index for the terminator, it just 
> >> doesn't write 0 to that byte. So the existing code already signals 
> >> an error in that case unless the `strz` is the final field.
> > 
> > I don't see how this is relevant to the concern I expressed.
> 
> The point I was trying to make is that this patch doesn't change the behavior 
> (in any significant way) in the case of an undersized output string. Perhaps 
> the documentation could be improved, but I'd rather do that in a follow-up 
> patch because it is outside of the scope of this patch.

I never had any objections to changing the behavior, nor in general to
the code changes in your patch.  My comments were only about the
documentation in the manual.

> > My concern is that you found it necessary to add a comment about 
> > writing the terminating null byte (which is a good thing), but didn't 
> > mention that aspect in the manual, not even as a hint.  I think it is 
> > noteworthy enough to be in the manual.
> 
> What do you mean?

I meant that the caveat about having enough space in the output string
for the terminating null byte is not explicitly mentioned where strz
and its handling during packing are documented.

> > Bottom line: I think this aspect of the code is important to mention 
> > in the manual.  The price is small, whereas the benefit could be 
> > significant.
> 
> I disagree -- I think the price is relatively high compared to the benefit. 
> The pre-allocated length requirement is a requirement of only `bindat-pack` 
> -- not of `bindat-type` or any of the type specifiers -- so it is best to 
> keep that requirement with the documentation of `bindat-pack`. Indeed, users 
> are unaware that packing to a pre-allocated string is even possible until 
> they read the documentation for `bindat-pack` (except for references in the 
> caveats documented for fixed-length `str` and `strz`, which I plan on 
> addressing in a future patch).

I don't see any point in continuing to argue about this.  I have my
opinions about how our manuals should explain these issues, and your
disagreement, which is noted, doesn't change those opinions.  So I
installed your previous version, and then made the changes in the
manual I thought were pertinent.

Thanks.