Consistent returns from ssnprintf/__ssnprintf wrappers, add ssnprintf2 for optional error handling. #3283

zebity · 2019-09-18T12:22:58Z

ChangeLog: collectd: Consistent ssnprintf/__ssnprintf returns, new ssnprintf2 for error handling.

@mrunge cherry picked fix for ssnprintf wrapper #3237 (mea culpa) back into master, thank you @faxm0dem . Applied same update to __ssnprintf wrapper in src/collectdctl.c for return result consistency and ensured opening comments consistent with code.

As per original suggestion from @octo (#2895) I have also added second wrapper ssnprintf2 into src/utils/common/common.c . This has the same return behaviour of original suggestion (which resulted in problem), so that in future if we want to add error handling for truncation (rather than ignore it) then this can be done on case by case basis.

The degree of impact of adding truncation error handling is to large to do in single hit, as I don't want to risk breaking code base a second time.

Thanks to all for continued updates into collectd.

Change-Id: Idab2c4ffa242a73b651f1b056f85f905e356c5ac

…ssnprintf wrapper

… on truncation in future

mrunge · 2019-09-19T13:38:46Z

Thank you, I'll try to get back to this asap.

rubenk · 2019-10-11T10:43:00Z

src/utils/common/common.c

@@ -89,8 +89,7 @@ char *sstrncpy(char *dest, const char *src, size_t n) {
  return dest;
 } /* char *sstrncpy */

-/* ssnprintf returns zero on success, one if truncation occurred
-   and a negative integer onerror. */
+/* ssnprintf returns result from vsnprintf conistent with snprintf */


so how is this function different then from snprintf?

Hi @rubenk the ssnprintf wrapper was introduced to get around GCC 8.2/.3 warning, only, there is no functional difference.
The original suggested fix included changing return to allow handling of truncation, however as this broke stuff.
So I to allow for future handling of truncation, case I added ssnprintf2 case.
Also to ensure that the other case that had the alternated return case (in collectdctl.c), I updated this to be consistent with snprintf.
As per discussion below, this was all driven by compiler behaviour rather than feature/fix need and yes the amount of code touched was greater than desired.
Cheers.

rubenk · 2019-10-11T10:44:36Z

src/utils/common/common.c

+
+/* ssnprintf2 returns zero on success, one if truncation occurred
+   and a negative integer on error. */
+int ssnprintf2(char *str, size_t sz, const char *format, ...) {


It seems that this does exactly the same as snprintf and ssnprintf, or am I missing something?

The return value is different, and I assume the idea is to provide both.

you can detect truncation with snprintf just fine, so I don't see how this helps.

rubenk · 2019-10-14T09:10:24Z

I'm sorry for perhaps sounding so negative, but this has all turned into a bit of a mess.
The original issue as I see it is two-fold:

gcc started warning for potential truncation.
we turn all warnings into errors, which breaks peoples builds.

Some of those truncation issues are real and we should fix them, some of them are not. Adding wrappers around them will never get them fixed. Admittingly checking every call site is a lot of work.

What I'd rather see is that we stop building with -Werror by default (most distros already turn that of). We could then turn -Werror on in CI, but turn the truncation errors into warnings (-Wno-error=stringop-truncation or something like that). We'll still see the warnings and can fix them in due time.

How does that sound?

mrunge · 2019-10-14T12:53:38Z

I agree, this sounds like a good plan to go forward.

So the next step(s) would be to revert the ssnprintf changes and to fix potential truncation.

Ideally, we'd remove ssnprintf and at the same time, fixing truncation warnings. I'd prefer to keep warnings as errors.

zebity · 2019-10-21T03:44:16Z

@rubenk & @mrunge,

sounds like a plan.

Wrapper was put in place due to number of places that -Werror broke build.

Remember that if you keep -Werror on in CI then all the builds will fail :-(

The amount of work required to get rid of warnings (assuming change the compile flags) is around approximately 67 files that where updated originally. It was the large number of files being changed that blocked getting this done.

I suspect that most of these files are dormant in terms of change so getting them fixed on an "as needs" basis will be slow process.

BTW, if we do turn off of -Werror with -W-Wno-error=stringop-truncation then its pretty easy to wind out all the auto-edits back to where they were, as I have record of all the original changs.

Cheers,

zebity

zebity · 2019-10-22T04:21:21Z

@mrunge @rubenk @rpv-tomsk

a quick find / grep indicates that there are 67 files with ssnprintf wrapper and 19 of these actually appear to use the return value.

So if any rework is going to be done then it would make sense to start with ones that use the return value, as these are potentially sensitive to truncation.

On different note, the number of plugins (there are around 60 libraries required to build for Debian package) and potential rate of change across these means that the release rate will have turn up substantially to allow collectd to keep up with changes forced to keep plugins current.

I think this is reflected by the very number of forked repositories.

Cheers,

Zebity.

mrunge · 2019-10-22T08:44:06Z

Thank you @zebity . What do you mean by "release rate"? Release rates for collectd? You may have seen #3318 ?

zebity · 2019-10-23T11:20:11Z

@mrunge yes to #3318 and I commend and thank you for your hard graft in keeping the wheels turning.

I was just doing some "out loud thinking...". I have been looking at the GitHub statistics and it would be really helpful to have some sense of rate of change across the entire code base (ie change with core vs change within pluggins and bug fix / currency driven change vs feature change.
I dwell on this due to my dear old debian packaging still languishing at 8.x ... :-(

Also I am a bit stunned by the 1000 or so forks... are these all mostly dormant or is there a whole lot of work happening outside of main branch ?

As I said I so just "thinking out loud" and will see if I can pull together some sort of statistical view.

EDIT: Also so I have reviewed the 5.10 open list and see this merge is one of items. My proposal at this point is remove the wrapper from the 19 functions that use the return (or least try to.., with appropriate caution and testing) and to leave the others us is (otherwise we will never get 5.10 out the door). Let me know your thoughts and I will do my bit of the work.

zebity · 2019-11-02T04:08:58Z

@rjablonx @mrunge I ask if you could please review and merge this request.

I have now wound out anything that is pre-emptive of immediate need (ie ssnprintf2 proposal), so change now just ensures that collectdctl.c is aligned with @faxm0dem fix (#3237).

I will not do any further changes related to the ssnprintf wrappers via this request.
This is In line with need to keep regular cadence as per discussion thread #3223
I believe that any further work on add/remove/tweeking build flags should be tackled and managed as individual merge requests and not a bulk change.

I think that is is consistent with approach of letting primary owner / maintainers of the various 67 files updated to be able to be involved in any further remediation.

For the initial identified set of 29, I believe the best approach would be equivalent 29 individual pull requests, which can run then there own course and with their own priority.
While this might seen onerous in terms of impact to release process, if we get greater level of automation going then it should allow ongoing smoother regular patch level releases, starting from new 5.10.0 release.

I will be good to get this initial bulk change (and associated formatting clean up) done and dusted.

Cheers,

Zebity

mrunge

Thank you for your work on this!

zebity · 2019-11-05T08:50:15Z

@mrunge many thanks.

FYI, I have updated test server to Ubuntu 19.10 so next step will be to test current 5.9.2/5.10.0 build against that and ensure all is ok on this update.

Cheers,

Zebity

Fᴀʙɪᴇɴ Wᴇʀɴʟɪ and others added 3 commits September 18, 2019 21:10

fix collectd#3232

8cae217

Change-Id: Idab2c4ffa242a73b651f1b056f85f905e356c5ac

Make return result consistent with fixed src/utils/common/common.c - …

092982d

…ssnprintf wrapper

Add second wrapper ssnprintf2 to allow introduction of error handling…

1b5a6b8

… on truncation in future

zebity changed the title ~~Provide consistent ssnprintf/__ssnprintf returns and add ssnprintf2 for optional error handling~~ Ensure consistent ssnprintf/__ssnprintf wrapper returns. Add ssnprintf2 for optional error handling. Sep 18, 2019

zebity changed the title ~~Ensure consistent ssnprintf/__ssnprintf wrapper returns. Add ssnprintf2 for optional error handling.~~ ChangeLog: ssnprintf wrappers: Ensure consistent returns, add ssnprintf2 for optional error handling. Sep 18, 2019

zebity changed the title ~~ChangeLog: ssnprintf wrappers: Ensure consistent returns, add ssnprintf2 for optional error handling.~~ Ensure consistent ssnprintf/__ssnprintf wrappers returns, add ssnprintf2 for optional error handling. Sep 18, 2019

zebity changed the title ~~Ensure consistent ssnprintf/__ssnprintf wrappers returns, add ssnprintf2 for optional error handling.~~ Consistent returns from ssnprintf/__ssnprintf wrappers, add ssnprintf2 for optional error handling. Sep 18, 2019

zebity mentioned this pull request Sep 18, 2019

rrdtool is broken in 5.9.1 #3232

Closed

Add ssnprintf2 wrapper variiant definition

4f6d6f7

mrunge added this to the 5.10.0 milestone Oct 11, 2019

rubenk reviewed Oct 11, 2019

View reviewed changes

Fix conflict from remove of ssnprintf2 and align utils/common/common.h

1137e9e

mrunge approved these changes Nov 2, 2019

View reviewed changes

mrunge merged commit 5c35a31 into collectd:master Nov 2, 2019

kkepka modified the milestones: 5.10.0, 5.11.0 Apr 3, 2020

kkepka mentioned this pull request May 11, 2020

Changelog from #3310 is missing in 5.11 release #3429

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consistent returns from ssnprintf/__ssnprintf wrappers, add ssnprintf2 for optional error handling. #3283

Consistent returns from ssnprintf/__ssnprintf wrappers, add ssnprintf2 for optional error handling. #3283

zebity commented Sep 18, 2019 •

edited

mrunge commented Sep 19, 2019

rubenk Oct 11, 2019

zebity Oct 21, 2019 •

edited

rubenk Oct 11, 2019

mrunge Oct 14, 2019

rubenk Oct 14, 2019

rubenk commented Oct 14, 2019

mrunge commented Oct 14, 2019

zebity commented Oct 21, 2019 •

edited

zebity commented Oct 22, 2019

mrunge commented Oct 22, 2019

zebity commented Oct 23, 2019 •

edited

zebity commented Nov 2, 2019 •

edited

mrunge left a comment

zebity commented Nov 5, 2019 •

edited

Consistent returns from ssnprintf/__ssnprintf wrappers, add ssnprintf2 for optional error handling. #3283

Consistent returns from ssnprintf/__ssnprintf wrappers, add ssnprintf2 for optional error handling. #3283

Conversation

zebity commented Sep 18, 2019 • edited

mrunge commented Sep 19, 2019

rubenk Oct 11, 2019

Choose a reason for hiding this comment

zebity Oct 21, 2019 • edited

Choose a reason for hiding this comment

rubenk Oct 11, 2019

Choose a reason for hiding this comment

mrunge Oct 14, 2019

Choose a reason for hiding this comment

rubenk Oct 14, 2019

Choose a reason for hiding this comment

rubenk commented Oct 14, 2019

mrunge commented Oct 14, 2019

zebity commented Oct 21, 2019 • edited

zebity commented Oct 22, 2019

mrunge commented Oct 22, 2019

zebity commented Oct 23, 2019 • edited

zebity commented Nov 2, 2019 • edited

mrunge left a comment

Choose a reason for hiding this comment

zebity commented Nov 5, 2019 • edited

zebity commented Sep 18, 2019 •

edited

zebity Oct 21, 2019 •

edited

zebity commented Oct 21, 2019 •

edited

zebity commented Oct 23, 2019 •

edited

zebity commented Nov 2, 2019 •

edited

zebity commented Nov 5, 2019 •

edited