[CRIU] [PATCH 4/4] unix: don't use %m in pr_warn

[Dmitry Safonov]

2018-05-16 0:25 GMT+01:00 Andrei Vagin <avagin at virtuozzo.com>:
> On Tue, May 15, 2018 at 04:22:15PM -0700, Andrei Vagin wrote:
>> On Wed, May 16, 2018 at 12:10:04AM +0100, Dmitry Safonov wrote:
>> > 2018-05-15 23:55 GMT+01:00 Andrei Vagin <avagin at virtuozzo.com>:
>> > > pr_warn calls a few system calls before printing a message,
>> > > so it will override errno.
>> > >
>> > > CID 190176 (#1 of 1): Printf format string issue (PW.BAD_PRINTF_FORMAT_STRING)
>> > > 1. bad_printf_format_string: invalid format string conversion
>> > >
>> > > Signed-off-by: Andrei Vagin <avagin at virtuozzo.com>
>> > > ---
>> > >  criu/sk-unix.c | 4 ++--
>> > >  1 file changed, 2 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/criu/sk-unix.c b/criu/sk-unix.c
>> > > index ea314bb9f..2838958d6 100644
>> > > --- a/criu/sk-unix.c
>> > > +++ b/criu/sk-unix.c
>> > > @@ -595,8 +595,8 @@ static int unix_resolve_name(int lfd, u32 id, struct unix_sk_desc *d,
>> > >         snprintf(rpath, sizeof(rpath), ".%s", name);
>> > >         if (fstatat(mntns_root, rpath, &st, 0)) {
>> > >                 if (errno != ENOENT) {
>> > > -                       pr_warn("Can't stat socket %#x(%s), skipping: %m (err %d)\n",
>> > > -                               id, rpath, errno);
>> > > +                       pr_warn("Can't stat socket %#x(%s), skipping: %s (err %d)\n",
>> > > +                               id, rpath, strerror(errno), errno);
>> >
>> > I don't get this one.
>> > pr_warn()=>print_on_level()=>vprint_on_level():
>> > :        int __errno = errno;
>> >
>> > Care to explain?
>>
>> vprint_on_level
>>       print_ts
>>               gettimeofday
>>               snprintf
>>               size  = vsnprintf(buffer + buf_off, sizeof buffer - buf_off, format, params);
>>
>>
>> gettimeofday() and snprintf() can change errno. We probably can fix this
>> by restoring errno before vsnprintf, but I would prefer to use stderror()
>> to avoid side effects.

Hmm, ok, but still I don't follow why fixing only one particular place then..

>> > I might not spot something obvious, then should we correct all pr_warn/pr_*,
>> > like the first in the same file:
>> > :                pr_warn("Unable to open a socket file: %m\n");
>> >
>> > Probably, Coverity just doesn't know %m modifier, not really sure if it worth
>> > to correct ALL %m in criu..
>>
>> I don't know why it shows this warning here, but I remember that we have
>> issues with %m...
>
> Acrually I know. In other cases, there is not other arguments except %m.
>
> For example:
>
> criu/cr-check.c:                        pr_msg("prctl: PR_SET_MM_EXE_FILE is not supported: %m\n");

Yeah, so probably Coverity expects %d for errno, but there is '%m (err %d)'
and it thinks we messed up with this.
If it's about fixing Coverity warning - maybe just swap them?
'(err %d): %m'

I do not feel strongly about the patch, anyway, may go as-is.

-- 
             Dmitry