Discussion:
[PATCH] Remove extra 0x in the %p formatting string
(too old to reply)
Timur I. Bakeyev via samba-technical
2018-05-16 13:38:34 UTC
Permalink
Raw Message
Hi Timur,
Hi all!
While analyzing debug logs of our customer, I've noticed strange looking
[2018/05/09 16:02:52.629315, 10, pid=907, effective(1001, 1001), real(0,
0)] ../source3/smbd/open.c:5543(create_file_default)
create_file: access_mask = 0x100080 file_attributes = 0x10,
share_access
= 0x7, create_disposition = 0x1 create_options = 0x1 oplock_request = 0x0
private_flags = 0x0 root_dir_fid = 0x0, ea_list = 0x0x0, sd = 0x0x0,
fname
= .
That 0x0x0 puzzled me and short investigation showed that in few places
we
use "0x%p" format string to represent buffer address in the debug logs.
Man
p The void * pointer argument is printed in hexadecimal
(as
if
by ‘%#x’ or ‘%#lx’).
I.e. the address will be already prefixed with the 0x, no need to
explicitly add it.
Here is a small patch.
Looks good.
For the changes in lib/dbwrap/dbwrap_tdb.c and source3/smbd/open.c, can
you please modernise the debug statements that you touch by changing
them as below?
DEBUG(10, (...));
to
DBG_DEBUG(...);
Thanks...
Thanks for the review, Martin!

Here are the patches that should fit you requirements, I hope.

List, can I get a second review, please :)?

With regards,
Timur Bakeyev.
Ralph Böhme via samba-technical
2018-05-16 16:06:48 UTC
Permalink
Raw Message
Post by Timur I. Bakeyev via samba-technical
List, can I get a second review, please :)?
lgtm. Pushed to autobuild.

Fwiw, can you send patchsets as git format-patch --stdout instead of one
attachment per patch? Thanks!

-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Franz Sirl via samba-technical
2018-05-16 16:18:24 UTC
Permalink
Raw Message
Post by Timur I. Bakeyev via samba-technical
Hi Timur,
Hi all!
While analyzing debug logs of our customer, I've noticed strange looking
[2018/05/09 16:02:52.629315, 10, pid=907, effective(1001, 1001), real(0,
0)] ../source3/smbd/open.c:5543(create_file_default)
create_file: access_mask = 0x100080 file_attributes = 0x10,
share_access
= 0x7, create_disposition = 0x1 create_options = 0x1 oplock_request = 0x0
private_flags = 0x0 root_dir_fid = 0x0, ea_list = 0x0x0, sd = 0x0x0,
fname
= .
That 0x0x0 puzzled me and short investigation showed that in few places
we
use "0x%p" format string to represent buffer address in the debug logs.
Man
p The void * pointer argument is printed in hexadecimal
(as
if
by ‘%#x’ or ‘%#lx’).
I.e. the address will be already prefixed with the 0x, no need to
explicitly add it.
Here is a small patch.
Looks good.
For the changes in lib/dbwrap/dbwrap_tdb.c and source3/smbd/open.c, can
you please modernise the debug statements that you touch by changing
them as below?
DEBUG(10, (...));
to
DBG_DEBUG(...);
Thanks...
Thanks for the review, Martin!
Here are the patches that should fit you requirements, I hope.
List, can I get a second review, please :)?
Note that %p is not portable, for example Solaris 10/11 don't prefix 0x
to the value. And glibc prints "(nil)" for a NULL-pointer.

Franz.
Timur I. Bakeyev via samba-technical
2018-05-16 19:59:54 UTC
Permalink
Raw Message
Note that %p is not portable, for example Solaris 10/11 don't prefix 0x to
the value. And glibc prints "(nil)" for a NULL-pointer.
I believe that %p is in the ANSI C standard. Solaris with it's own very
special C compiler.. Unfortunately, standard doesn't really define, how the
pointer should look like. Hence lack of 0x in some implementations and
(nil). As for the last one, I believe that

buffer is: (nil)

still looks better than:

buffer is: 0x(nil)

If we want even more portability we may employ PRIxPTR macro from
stdint.h/inttypes.h(which didn't present in Solaris 2.8 at least :>) and
cast to uintptr_t. But not sure we need such a complications.

With regards,
Timur Bakeyev.
Andrew Bartlett via samba-technical
2018-05-16 19:42:18 UTC
Permalink
Raw Message
On Wed, 2018-05-16 at 18:18 +0200, Franz Sirl via samba-technical
Post by Franz Sirl via samba-technical
Note that %p is not portable, for example Solaris 10/11 don't prefix 0x
to the value. And glibc prints "(nil)" for a NULL-pointer.
A patch to lib/replace/tests/snprintf.c would be a good idea then. We
can then replace printf on these platforms.

Andrew Bartlett
--
Andrew Bartlett http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org
Samba Developer, Catalyst IT http://catalyst.net.nz/services/samba
Loading...