Skip to content

Fix output messages#84

Merged
anderslindho merged 4 commits intoepics-modules:masterfrom
lucmaga:fix_newlines
Nov 18, 2025
Merged

Fix output messages#84
anderslindho merged 4 commits intoepics-modules:masterfrom
lucmaga:fix_newlines

Conversation

@lucmaga
Copy link
Contributor

@lucmaga lucmaga commented Oct 9, 2025

  • Add missing newlines
  • Use errlogPrintf for errors

fixes #81

Copy link
Collaborator

@simon-ess simon-ess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seem to be a few also in the function save_restoreShow, see

            epicsTimeToStrftime(tmpstr, sizeof(tmpstr), TIMEFMT, &plist->save_time);
            printf("    Last save time  :%s", tmpstr);
            epicsTimeToStrftime(tmpstr, sizeof(tmpstr), TIMEFMT, &plist->backup_time);
printf(" Last backup time:%s", tmpstr);

and

printf(" channel_connected = %d", pchannel->channel_connected);

@anjohnson
Copy link
Member

Please don't mindlessly convert every printf() call into errlogPrintf() without looking at them carefully.

I'm not sure whether you have or not since there are so many changes here and I don't know the code in detail, but any routines that are normally run by an IOCSH command (possibly indirectly) may be deliberately using printf() so those messages are displayed synchronously with the command execution. Printing shell-command error messages through errlog can cause them to be interleaved with other command outputs and get out of order, which gets confusing to the user. I'm specifically looking at the "manual" routines (there are many IOCSH commands with that in the name), although I don't know whether they can be triggered from a PV or are just called from IOCSH commands. That might apply to other routines too though.

Please respond and confirm whether you were aware of and have considered this subtlety.

Also, the filename "log.h" makes me a bit nervous given how generic that is, although I believe it should always be found before any OS-specific "log.h" file since you're using double-quotes in the #include statement for it.

@lucmaga
Copy link
Contributor Author

lucmaga commented Oct 29, 2025

Please don't mindlessly convert every printf() call into errlogPrintf() without looking at them carefully.

I'm not sure whether you have or not since there are so many changes here and I don't know the code in detail, but any routines that are normally run by an IOCSH command (possibly indirectly) may be deliberately using printf() so those messages are displayed synchronously with the command execution. Printing shell-command error messages through errlog can cause them to be interleaved with other command outputs and get out of order, which gets confusing to the user. I'm specifically looking at the "manual" routines (there are many IOCSH commands with that in the name), although I don't know whether they can be triggered from a PV or are just called from IOCSH commands. That might apply to other routines too though.

Please respond and confirm whether you were aware of and have considered this subtlety.

Thanks for the review. The commits 59d6185 and 59736ee are the ones I convert printf to errlogPrintf. I kept them separate for easier review.
I was careful to convert only messages where there was a branch checking on error, a return ERROR like statement. Specially save_restore.c is a tricky one to convert, I'm not sure I cover all error outputs but most of them, at least.

Also, the filename "log.h" makes me a bit nervous given how generic that is, although I believe it should always be found before any OS-specific "log.h" file since you're using double-quotes in the #include statement for it.

Funny, I was expecting this. I'll try to think into a better name.

@lucmaga lucmaga requested a review from simon-ess October 29, 2025 09:43
Copy link
Collaborator

@simon-ess simon-ess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is overall a good change, although as @anjohnson pointed out there are a few of the errlog changes that perhaps should remain printfs (in the manual restore function).

The error logs had a mixture of standards in regard of preappending
tags like file, function or just save_restore. This commit adds a macro
and convert all error output to use it so all messages have the same
format.
@simon-ess
Copy link
Collaborator

LGTM now. Maybe add a release note about this, noting that anyone parsing their IOC logs (e.g. ESS 😄 ) for saverestore output may want to revisit that parsing...

@lucmaga
Copy link
Contributor Author

lucmaga commented Nov 17, 2025

Hi, is there anything missing for this to be merged?

@anderslindho anderslindho merged commit 0189397 into epics-modules:master Nov 18, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Not every errlogprintf command has an EOL character

4 participants