Re: [Tarantool-patches] [PATCH] say: fix format for fiber()->fid

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Tue, Feb 23, 2021 at 09:41:46PM +0100, Vladislav Shpilevoy wrote:
> Hi! Please, add a changelog file. This is user-visible behaviour
> so it must be reflected in the changes.
> 
> On 22.02.2021 19:20, Cyrill Gorcunov wrote:
> > The fiber's ID (fiber::fid) is unsigned integer so
> > we should use a proper format specificator when printing
> > it out, otherwise the logger show us weird strings like
> > 
> >  | main/-244760339/cartridge.failover.task I> Instance state changed
> > 
> > Fixes #5846
> > 
> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> > ---
> >  src/lib/core/say.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/lib/core/say.c b/src/lib/core/say.c
> > index cbd10e107..6138752a7 100644
> > --- a/src/lib/core/say.c
> > +++ b/src/lib/core/say.c
> > @@ -792,7 +792,7 @@ say_format_plain_tail(char *buf, int len, int level, const char *filename,
> >  	if (cord) {
> >  		SNPRINT(total, snprintf, buf, len, " %s", cord->name);
> >  		if (fiber() && fiber()->fid != FIBER_ID_SCHED) {
> > -			SNPRINT(total, snprintf, buf, len, "/%i/%s",
> > +			SNPRINT(total, snprintf, buf, len, "/%u/%s",
> >  				fiber()->fid, fiber_name(fiber()));
> 
> I remember we had some issues about %u not being compatible with uint32_t so
> we did manual casts to 'unsigned'. Not sure though what was the commit/ticket,
> or if it really happened.

I suspect you rather mean uint64_t, which indeed better convert to
excplicit "long long". Strictly speaking if we really need to cover
compatibility we would _had_ to use PRIx macros which are so ugly :/
So no, uint32_t is known to be safe with %u format.

Side note: I found that we still use %u for some of uint64_t,
in particuar

	if (req->term < raft->volatile_term) {
		say_info("RAFT: the message is ignored due to outdated term - "
			 "current term is %u", raft->volatile_term);
		return 0;
	}

probably should use %llu and explicit "long long" for long standing
servers where raft term would overflow 32 bit value.

> 
> I am fine with not using the cast as long as all platforms and compilers
> build flawlessly. Also it is ok for me if you will add an explicit cast in
> the next version of the patch. Up to you. The same below.

No, Vlad, I think we don't need explicit casts for 32bit values. I pushed
an update to the branch gorcunov/gh-5846-fid-name and here is a new
changelog. Does it look better?
---
Subject: [PATCH] say: use unsigned  format for fiber()->fid

The fiber's ID (fiber::fid) is defined as unsigned integer
so we should use a proper format specificator when printing
it out. Currently logger prints such fibers as negative

 | main/-244760339/cartridge.failover.task I> Instance state changed

why it should be

 | main/4271292179/cartridge.failover.task I> Instance state changed

After the fix all fibers are logged with natural numbers (both in
plain logger and json output formats) which might be considered
as API change but should not cause any problems because if there
were tools which scans for "zahlen" numbers they will scan for
natural numbers without problems. After all there is no such
things as fibers with negative numbers and when their fids
are printed in Lua's "info" they are all natural (because
they are converted into real numbers internally).

Fixes #5846