3:23 a.m.
I've managed to instigate clang to build some pieces
of crash-utility code, and it showed unpleasant things
which I'd like to bring into your attention.
The patch bomb contains only minor nitpicks to check
whether I should continue with doing this kind of nasty job
further using static analysis tools, various compilers
and common sense.
Oleksandr Natalenko (14):
tools: fix non-literal NULL conversion
memory: do not compare unsigned expression with negative value
memory: use & for a bitwise operation
task: remove extra parentheses in equality comparison
x86_64: clarify logical operator precedence
lkcd_v1: address of an array always evaluates to true
lkcd_v2_v3: address of an array always evaluates to true
lkcd_v5: address of an array always evaluates to true
lkcd_v7: address of an array always evaluates to true
lkcd_v8: address of an array always evaluates to true
qemu-load: remove unused function get_le64()
sadump: block_size is always non-negative
task: format string should be never empty
symbols: format string should be never empty
lkcd_v1.c | 2 +-
lkcd_v2_v3.c | 2 +-
lkcd_v5.c | 2 +-
lkcd_v7.c | 2 +-
lkcd_v8.c | 2 +-
memory.c | 8 ++++----
qemu-load.c | 8 --------
sadump.c | 3 ---
symbols.c | 9 +++++----
task.c | 20 +++++++++++---------
tools.c | 2 +-
x86_64.c | 4 ++--
12 files changed, 28 insertions(+), 36 deletions(-)
--
2.14.3
3:23 a.m.
New subject: [PATCH RFC 01/14] tools: fix non-literal NULL conversion
Signed-off-by: Oleksandr Natalenko <oleksandr(a)redhat.com>
---
tools.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools.c b/tools.c
index 186b703..97859ae 100644
--- a/tools.c
+++ b/tools.c
@@ -246,7 +246,7 @@ next:
str[i] = NULLCHAR;
/* keep falling... */
case NULLCHAR:
- argv[j] = NULLCHAR;
+ argv[j] = NULL;
return(j);
}
}
--
2.14.3
3:23 a.m.
New subject: [PATCH RFC 02/14] memory: do not compare unsigned expression with negative value
Signed-off-by: Oleksandr Natalenko <oleksandr(a)redhat.com>
---
memory.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/memory.c b/memory.c
index 9926199..abf3982 100644
--- a/memory.c
+++ b/memory.c
@@ -14467,7 +14467,7 @@ display_with_pre_and_post(void *bufptr, ulonglong addr, struct
searchinfo *si)
}
amount = ctx * t;
- addr_d = addr - amount < 0 ? 0 : addr - amount;
+ addr_d = (long)addr - amount < 0 ? 0 : addr - amount;
display_memory(addr_d, ctx, flag, memtype, NULL);
--
2.14.3
2:56 p.m.
New subject: [PATCH RFC 02/14] memory: do not compare unsigned expression with negative value
----- Original Message -----
Signed-off-by: Oleksandr Natalenko <oleksandr(a)redhat.com>
---
memory.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/memory.c b/memory.c
index 9926199..abf3982 100644
--- a/memory.c
+++ b/memory.c
@@ -14467,7 +14467,7 @@ display_with_pre_and_post(void *bufptr, ulonglong
addr, struct searchinfo *si)
}
amount = ctx * t;
- addr_d = addr - amount < 0 ? 0 : addr - amount;
+ addr_d = (long)addr - amount < 0 ? 0 : addr - amount;
display_memory(addr_d, ctx, flag, memtype, NULL);
--
I didn't write this code, but I do take the responsibility for accepting the patch.
The code above is used by the "search" command, where if it finds an instance of
the
specified searched-for value, it will not only display the memory location where it
exists,
but also the context around it. The context is a "-x <count>" argument
value passed to
the "search" command, which is multiplied by the word size specified. I'm
not sure
what the originator of this code meant by the "addr - amount < 0"
qualification,
because any kernel virtual address (other than s390x) would be perceived as a negative
number when preceded by "(long)". So if your patch is applied, it would always
be < 0,
set addr_d to 0, which gets ignored by display_memory(), and therefore it would never
show the context preceding the found entity as intended.
So again, you have found a coding irregularity, but your patch breaks what works just
fine.
Probably it should just set addr_d = addr - amount, but that's what the code
effectively
does now (by mistake), so I'm just going to leave this alone.
Dave
1:50 p.m.
New subject: [PATCH RFC 02/14] memory: do not compare unsigned expression with negative value
----- Original Message -----
Signed-off-by: Oleksandr Natalenko <oleksandr(a)redhat.com>
---
memory.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/memory.c b/memory.c
index 9926199..abf3982 100644
--- a/memory.c
+++ b/memory.c
@@ -14467,7 +14467,7 @@ display_with_pre_and_post(void *bufptr, ulonglong
addr, struct searchinfo *si)
}
amount = ctx * t;
- addr_d = addr - amount < 0 ? 0 : addr - amount;
+ addr_d = (long)addr - amount < 0 ? 0 : addr - amount;
display_memory(addr_d, ctx, flag, memtype, NULL);
--
2.14.3
> Probably it should just set addr_d = addr - amount, but
that's what the code effectively
> does now (by mistake), so I'm just going to leave this alone.
Would you, maybe, still prefer modifying it to addr_d = addr - amount
anyway to avoid both compiler and code reviewer confusion in the future please?
OK fine, queued for crash-7.2.1:
https://github.com/crash-utility/crash/commit/b2d1bba766118fddf43235f0bed...
Dave
3:23 a.m.
New subject: [PATCH RFC 03/14] memory: use & for a bitwise operation
Signed-off-by: Oleksandr Natalenko <oleksandr(a)redhat.com>
---
memory.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/memory.c b/memory.c
index abf3982..60106d2 100644
--- a/memory.c
+++ b/memory.c
@@ -17003,8 +17003,8 @@ valid_section(ulong addr)
if ((mem_section = read_mem_section(addr)))
return (ULONG(mem_section +
- OFFSET(mem_section_section_mem_map)) &&
- SECTION_MARKED_PRESENT);
+ OFFSET(mem_section_section_mem_map))
+ & SECTION_MARKED_PRESENT);
return 0;
}
@@ -17016,7 +17016,7 @@ section_has_mem_map(ulong addr)
if ((mem_section = read_mem_section(addr)))
return (ULONG(mem_section +
OFFSET(mem_section_section_mem_map))
- && SECTION_HAS_MEM_MAP);
+ & SECTION_HAS_MEM_MAP);
return 0;
}
--
2.14.3
9:15 a.m.
----- Original Message -----
I'll take a look at these when I get the chance, but I'm really
not particularly excited unless they are actual bugs.
Like this one:
--- a/memory.c
+++ b/memory.c
@@ -17003,8 +17003,8 @@ valid_section(ulong addr)
if ((mem_section = read_mem_section(addr)))
return (ULONG(mem_section +
- OFFSET(mem_section_section_mem_map)) &&
- SECTION_MARKED_PRESENT);
+ OFFSET(mem_section_section_mem_map))
+ & SECTION_MARKED_PRESENT);
return 0;
}
@@ -17016,7 +17016,7 @@ section_has_mem_map(ulong addr)
if ((mem_section = read_mem_section(addr)))
return (ULONG(mem_section +
OFFSET(mem_section_section_mem_map))
- && SECTION_HAS_MEM_MAP);
+ & SECTION_HAS_MEM_MAP);
return 0;
}
This code has been like this since the original CONFIG_SPARSEMEM support
patch was posted back in 2006. Interesting that this has never been a
problem. Apparently nobody's ever bumped into mem_section that didn't
have those flags.
And then there's this one:
--- a/x86_64.c
+++ b/x86_64.c
@@ -2086,7 +2086,7 @@ x86_64_kvtop(struct task_context *tc, ulong kvaddr, physaddr_t
*paddr, int verbo
}
start_vtop_with_pagetable:
- if (!(*pml4) & _PAGE_PRESENT)
+ if ((!(*pml4)) & _PAGE_PRESENT)
goto no_kpage;
pgd_paddr = (*pml4) & PHYSICAL_PAGE_MASK;
FILL_PGD(pgd_paddr, PHYSADDR, PAGESIZE());
@@ -2187,7 +2187,7 @@ x86_64_kvtop_xen_wpt(struct task_context *tc, ulong kvaddr,
physaddr_t *paddr, i
fprintf(fp, "PML4 DIRECTORY: %lx\n", vt->kernel_pgd[0]);
fprintf(fp, "PAGE DIRECTORY: %lx [machine]\n", *pml4);
}
- if (!(*pml4) & _PAGE_PRESENT)
+ if ((!(*pml4)) & _PAGE_PRESENT)
goto no_kpage;
pgd_paddr = (*pml4) & PHYSICAL_PAGE_MASK;
pgd_paddr = xen_m2p(pgd_paddr);
It's pretty much impossible to have a pml without its PAGE_PRESENT bit set,
so it's been a benign bug. But the fix is incorrect. They should be:
if (!(*pml4 & _PAGE_PRESENT))
And I'm not sure wheter this one should be fixed by just removing the statement:
--- a/sadump.c
+++ b/sadump.c
@@ -157,9 +157,6 @@ read_dump_header(char *file)
}
restart:
- if (block_size < 0)
- return FALSE;
-
if (!read_device(sph, block_size, &offset)) {
error(INFO, "sadump: cannot read partition header\n");
goto err;
because farther down there is this:
if (sh->block_size != block_size) {
block_size = sh->block_size;
offset = 0;
goto restart;
}
I'll let the sadump maintainer decide how to deal with this one. He's on
this list, but I've cc'd him to get his attention.
Thanks,
Dave
10:32 a.m.
----- Original Message -----
----- Original Message -----
>
> I'll take a look at these when I get the chance, but I'm really
> not particularly excited unless they are actual bugs.
Like this one:
--- a/memory.c
+++ b/memory.c
@@ -17003,8 +17003,8 @@ valid_section(ulong addr)
if ((mem_section = read_mem_section(addr)))
return (ULONG(mem_section +
- OFFSET(mem_section_section_mem_map)) &&
- SECTION_MARKED_PRESENT);
+ OFFSET(mem_section_section_mem_map))
+ & SECTION_MARKED_PRESENT);
return 0;
}
@@ -17016,7 +17016,7 @@ section_has_mem_map(ulong addr)
if ((mem_section = read_mem_section(addr)))
return (ULONG(mem_section +
OFFSET(mem_section_section_mem_map))
- && SECTION_HAS_MEM_MAP);
+ & SECTION_HAS_MEM_MAP);
return 0;
}
This code has been like this since the original CONFIG_SPARSEMEM support
patch was posted back in 2006. Interesting that this has never been a
problem. Apparently nobody's ever bumped into mem_section that didn't
have those flags.
Interestingly enough, this patch breaks RHEL5 and earlier kernels.
(try "kmem -n" with and without the patch). Probably a flag issue with
earlier CONFIG_SPARSEMEM kernels.
Dave
1:21 p.m.
----- Original Message -----
----- Original Message -----
> ----- Original Message -----
> >
> > I'll take a look at these when I get the chance, but I'm really
> > not particularly excited unless they are actual bugs.
>
> Like this one:
>
> --- a/memory.c
> +++ b/memory.c
> @@ -17003,8 +17003,8 @@ valid_section(ulong addr)
>
> if ((mem_section = read_mem_section(addr)))
> return (ULONG(mem_section +
> - OFFSET(mem_section_section_mem_map)) &&
> - SECTION_MARKED_PRESENT);
> + OFFSET(mem_section_section_mem_map))
> + & SECTION_MARKED_PRESENT);
> return 0;
> }
>
> @@ -17016,7 +17016,7 @@ section_has_mem_map(ulong addr)
> if ((mem_section = read_mem_section(addr)))
> return (ULONG(mem_section +
> OFFSET(mem_section_section_mem_map))
> - && SECTION_HAS_MEM_MAP);
> + & SECTION_HAS_MEM_MAP);
> return 0;
> }
>
> This code has been like this since the original CONFIG_SPARSEMEM support
> patch was posted back in 2006. Interesting that this has never been a
> problem. Apparently nobody's ever bumped into mem_section that didn't
> have those flags.
Interestingly enough, this patch breaks RHEL5 and earlier kernels.
(try "kmem -n" with and without the patch). Probably a flag issue with
earlier CONFIG_SPARSEMEM kernels.
Dave
OK, mystery solved. Prior to 2.6.24, the SECTION_HAS_MEM_MAP bit existed,
but it was never set/used in the mem_section.section_mem_map combination
pointer/bitmask. So when run against the early kernels, the patched version
of section_has_mem_map() above would fail because only the SECTION_MARKED_PRESENT
bit was ever set.
However, for all practical purposes, if a mem_section exists, it is valid, so
even without the patch, things have always worked just fine with the coding
error in place.
Regardless, I fixed the patch to be based upon the kernel version, and queued
it for crash-7.1.2:
https://github.com/crash-utility/crash/commit/e81db08bc69fb1a7a7e48f892c2...
Dave
2:41 a.m.
-----Original Message-----
From: Dave Anderson [mailto:anderson@redhat.com]
Sent: Friday, October 27, 2017 11:16 PM
To: Oleksandr Natalenko <oleksandr(a)redhat.com>
Cc: crash-utility(a)redhat.com; Hatayama, Daisuke
<d.hatayama(a)jp.fujitsu.com>; d hatayama <d.hatayama(a)gmail.com>
Subject: Re: [PATCH RFC 00/14] Minor code cleanups, round zero
----- Original Message -----
>
> I'll take a look at these when I get the chance, but I'm really
> not particularly excited unless they are actual bugs.
Like this one:
--- a/memory.c
+++ b/memory.c
@@ -17003,8 +17003,8 @@ valid_section(ulong addr)
if ((mem_section = read_mem_section(addr)))
return (ULONG(mem_section +
- OFFSET(mem_section_section_mem_map)) &&
- SECTION_MARKED_PRESENT);
+ OFFSET(mem_section_section_mem_map))
+ & SECTION_MARKED_PRESENT);
return 0;
}
@@ -17016,7 +17016,7 @@ section_has_mem_map(ulong addr)
if ((mem_section = read_mem_section(addr)))
return (ULONG(mem_section +
OFFSET(mem_section_section_mem_map))
- && SECTION_HAS_MEM_MAP);
+ & SECTION_HAS_MEM_MAP);
return 0;
}
This code has been like this since the original CONFIG_SPARSEMEM support
patch was posted back in 2006. Interesting that this has never been a
problem. Apparently nobody's ever bumped into mem_section that didn't
have those flags.
And then there's this one:
--- a/x86_64.c
+++ b/x86_64.c
@@ -2086,7 +2086,7 @@ x86_64_kvtop(struct task_context *tc, ulong kvaddr,
physaddr_t *paddr, int verbo
}
start_vtop_with_pagetable:
- if (!(*pml4) & _PAGE_PRESENT)
+ if ((!(*pml4)) & _PAGE_PRESENT)
goto no_kpage;
pgd_paddr = (*pml4) & PHYSICAL_PAGE_MASK;
FILL_PGD(pgd_paddr, PHYSADDR, PAGESIZE());
@@ -2187,7 +2187,7 @@ x86_64_kvtop_xen_wpt(struct task_context *tc, ulong
kvaddr, physaddr_t *paddr, i
fprintf(fp, "PML4 DIRECTORY: %lx\n", vt->kernel_pgd[0]);
fprintf(fp, "PAGE DIRECTORY: %lx [machine]\n", *pml4);
}
- if (!(*pml4) & _PAGE_PRESENT)
+ if ((!(*pml4)) & _PAGE_PRESENT)
goto no_kpage;
pgd_paddr = (*pml4) & PHYSICAL_PAGE_MASK;
pgd_paddr = xen_m2p(pgd_paddr);
It's pretty much impossible to have a pml without its PAGE_PRESENT bit set,
so it's been a benign bug. But the fix is incorrect. They should be:
if (!(*pml4 & _PAGE_PRESENT))
And I'm not sure wheter this one should be fixed by just removing the statement:
--- a/sadump.c
+++ b/sadump.c
@@ -157,9 +157,6 @@ read_dump_header(char *file)
}
restart:
- if (block_size < 0)
- return FALSE;
-
if (!read_device(sph, block_size, &offset)) {
error(INFO, "sadump: cannot read partition header\n");
goto err;
because farther down there is this:
if (sh->block_size != block_size) {
block_size = sh->block_size;
offset = 0;
goto restart;
}
I'll let the sadump maintainer decide how to deal with this one. He's on
this list, but I've cc'd him to get his attention.
Thanks for telling me this, Dave.
Oleksandr, this looks good to me.
Thanks for your patch.
block_size is of type size_t and size_t is always defined as
some unsigned integer type in the C standards.
So, the condition on block_size is meaningless and can be removed.
Thanks,
Dave
4:28 a.m.
Hi, Dave, Daisuke.
OK, mystery solved.
…
Regardless, I fixed the patch to be based upon the kernel version, and queued
it for crash-7.1.2:
https://github.com/crash-utility/crash/commit/e81db08bc69fb1a7a7e48f892c2...
Thanks for looking into it, now it makes more sense to me.
Probably it should just set addr_d = addr - amount, but that's
what the code effectively
does now (by mistake), so I'm just going to leave this alone.
Would you, maybe, still prefer modifying it to addr_d = addr - amount
anyway to avoid both
compiler and code reviewer confusion in the future please?
It's pretty much impossible to have a pml without its
PAGE_PRESENT bit set,
so it's been a benign bug. But the fix is incorrect. They should be:
if (!(*pml4 & _PAGE_PRESENT))
Thanks for the explanation. Would you still prefer changing it to a
proper expression
since original one is confusing (NOT is evaluated before &)?
Oleksandr, this looks good to me.
Thanks for your patch.
Thank you for looking into it. Dave, would you prefer applying this
patch directly now?
Also, I'd still like to get a response regarding other patches in the
series once you have
some time.
Thanks!
2:10 p.m.
----- Original Message -----
Hi, Dave, Daisuke.
> OK, mystery solved.
> …
> Regardless, I fixed the patch to be based upon the kernel version, and queued
> it for crash-7.1.2:
>
>
https://github.com/crash-utility/crash/commit/e81db08bc69fb1a7a7e48f892c2...
Thanks for looking into it, now it makes more sense to me.
> Probably it should just set addr_d = addr - amount, but that's what the code
effectively
> does now (by mistake), so I'm just going to leave this alone.
Would you, maybe, still prefer modifying it to addr_d = addr - amount anyway to avoid
both
compiler and code reviewer confusion in the future please?
> It's pretty much impossible to have a pml without its PAGE_PRESENT bit set,
> so it's been a benign bug. But the fix is incorrect. They should be:
>
> if (!(*pml4 & _PAGE_PRESENT))
Thanks for the explanation. Would you still prefer changing it to a proper expression
since original one is confusing (NOT is evaluated before &)?
> Oleksandr, this looks good to me.
> Thanks for your patch.
Thank you for looking into it. Dave, would you prefer applying this
patch directly now?
Also, I'd still like to get a response regarding other patches in the
series once you have some time.
Yeah, as I mentioned before, I am interested in fixing actual bugs that could
conceivably pose a real-life problem.
Dave
2:30 a.m.
Hi, Dave.
Thanks for your intensive review and accepting contribution.
Nevertheless, I'd appreciate if you explicitly comment on the following series:
lkcd_v8: address of an array always evaluates to true
lkcd_v7: address of an array always evaluates to true
lkcd_v5: address of an array always evaluates to true
lkcd_v2_v3: address of an array always evaluates to true
lkcd_v1: address of an array always evaluates to true
Let me explain why I think this should be fixed.
dump_header_t is a struct that contains the following member:
/* the panic string, if available */
char dh_panic_string[DUMP_PANIC_LEN];
This struct is statically initialized with zeroes:
static dump_header_t dump_header_v8 = { 0 };
and assigned to dh via pointer. Thus, in the following expression:
dh && dh->dh_panic_string && strstr(dh->dh_panic_string,
"\n") ? "" : "\n"
once dh is non-NULL, dh->dh_panic_string also has some address within
a struct, but its value may be NULL. IIRC, passing NULL to strstr is
an undefined behaviour, so, looks like that this statement should be
rewritten to check whether dh_panic_string is NULL:
dh && *dh->dh_panic_string && strstr(dh->dh_panic_string,
"\n") ? "" : "\n"
or
dh && dh->dh_panic_string[0] && strstr(dh->dh_panic_string,
"\n") ? "" : "\n"
or even
dh && strlen(dh->dh_panic_string) && strstr(dh->dh_panic_string,
"\n")
? "" : "\n"
to avoid any confusion for potential code reviewer.
Am I correct?
Thanks.
On Mon, Oct 30, 2017 at 8:10 PM, Dave Anderson <anderson(a)redhat.com> wrote:
----- Original Message -----
> Hi, Dave, Daisuke.
>
> > OK, mystery solved.
> > …
> > Regardless, I fixed the patch to be based upon the kernel version, and queued
> > it for crash-7.1.2:
> >
> >
https://github.com/crash-utility/crash/commit/e81db08bc69fb1a7a7e48f892c2...
>
> Thanks for looking into it, now it makes more sense to me.
>
> > Probably it should just set addr_d = addr - amount, but that's what the code
effectively
> > does now (by mistake), so I'm just going to leave this alone.
>
> Would you, maybe, still prefer modifying it to addr_d = addr - amount anyway to avoid
both
> compiler and code reviewer confusion in the future please?
>
> > It's pretty much impossible to have a pml without its PAGE_PRESENT bit set,
> > so it's been a benign bug. But the fix is incorrect. They should be:
> >
> > if (!(*pml4 & _PAGE_PRESENT))
>
> Thanks for the explanation. Would you still prefer changing it to a proper
expression
> since original one is confusing (NOT is evaluated before &)?
>
> > Oleksandr, this looks good to me.
> > Thanks for your patch.
>
> Thank you for looking into it. Dave, would you prefer applying this
> patch directly now?
>
> Also, I'd still like to get a response regarding other patches in the
> series once you have some time.
Yeah, as I mentioned before, I am interested in fixing actual bugs that could
conceivably pose a real-life problem.
Dave
--
Best regards,
Oleksandr Natalenko (post-factum)
Software Maintenance Engineer
9:34 a.m.
----- Original Message -----
Hi, Dave.
Thanks for your intensive review and accepting contribution.
Nevertheless, I'd appreciate if you explicitly comment on the following
series:
lkcd_v8: address of an array always evaluates to true
lkcd_v7: address of an array always evaluates to true
lkcd_v5: address of an array always evaluates to true
lkcd_v2_v3: address of an array always evaluates to true
lkcd_v1: address of an array always evaluates to true
Let me explain why I think this should be fixed.
dump_header_t is a struct that contains the following member:
/* the panic string, if available */
char dh_panic_string[DUMP_PANIC_LEN];
This struct is statically initialized with zeroes:
static dump_header_t dump_header_v8 = { 0 };
and assigned to dh via pointer. Thus, in the following expression:
dh && dh->dh_panic_string && strstr(dh->dh_panic_string,
"\n") ? "" : "\n"
once dh is non-NULL, dh->dh_panic_string also has some address within
a struct, but its value may be NULL. IIRC, passing NULL to strstr is
an undefined behaviour, so, looks like that this statement should be
rewritten to check whether dh_panic_string is NULL:
dh && *dh->dh_panic_string && strstr(dh->dh_panic_string,
"\n") ? "" : "\n"
or
dh && dh->dh_panic_string[0] && strstr(dh->dh_panic_string,
"\n") ? "" : "\n"
or even
dh && strlen(dh->dh_panic_string) && strstr(dh->dh_panic_string,
"\n")
? "" : "\n"
to avoid any confusion for potential code reviewer.
Am I correct?
Yes you are correct that the dh->dh_panic_string test will always
return TRUE, but that it's harmless. And it would never pass a NULL
pointer to strstr(), but rather a pointer to a string of NULL characters,
i.e., a legitimate string whose terminating NULL is the first character.
And FWIW, it's also meaningless to even make the "dh" check as well,
so the two-part "dh && dh->dh_panic_string" validation could simply
be removed.
But practically speaking -- and I don't mean to be flippant -- when
was the last time you worked with an LKCD dumpfile? It's a dead dumpfile
format, that was abandoned in ~2006. I think SUSE last used it in their
2.6.5-based SLES9 kernels, but thankfully kdump was accepted in the kernel,
and LKCD (and Red Hat's netdump) were finally and thankfully laid to rest.
Again, what I'm interested in is just keeping crash working/in-sync
with the constant onslaught of upstream kernel changes, fixing any existing
functional bugs, and adding new useful features.
Thanks,
Dave
3:23 a.m.
New subject: [PATCH RFC 04/14] task: remove extra parentheses in equality comparison
Signed-off-by: Oleksandr Natalenko <oleksandr(a)redhat.com>
---
task.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/task.c b/task.c
index 5754159..261ca31 100644
--- a/task.c
+++ b/task.c
@@ -3384,7 +3384,7 @@ show_ps(ulong flag, struct psinfo *psi)
break;
case PS_BY_TASK:
- if ((tc->task == psi->task[ac]))
+ if (tc->task == psi->task[ac])
print = TRUE;
break;
--
2.14.3
3:23 a.m.
New subject: [PATCH RFC 05/14] x86_64: clarify logical operator precedence
Signed-off-by: Oleksandr Natalenko <oleksandr(a)redhat.com>
---
x86_64.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/x86_64.c b/x86_64.c
index 2f9e6db..7b25e60 100644
--- a/x86_64.c
+++ b/x86_64.c
@@ -2086,7 +2086,7 @@ x86_64_kvtop(struct task_context *tc, ulong kvaddr, physaddr_t
*paddr, int verbo
}
start_vtop_with_pagetable:
- if (!(*pml4) & _PAGE_PRESENT)
+ if ((!(*pml4)) & _PAGE_PRESENT)
goto no_kpage;
pgd_paddr = (*pml4) & PHYSICAL_PAGE_MASK;
FILL_PGD(pgd_paddr, PHYSADDR, PAGESIZE());
@@ -2187,7 +2187,7 @@ x86_64_kvtop_xen_wpt(struct task_context *tc, ulong kvaddr,
physaddr_t *paddr, i
fprintf(fp, "PML4 DIRECTORY: %lx\n", vt->kernel_pgd[0]);
fprintf(fp, "PAGE DIRECTORY: %lx [machine]\n", *pml4);
}
- if (!(*pml4) & _PAGE_PRESENT)
+ if ((!(*pml4)) & _PAGE_PRESENT)
goto no_kpage;
pgd_paddr = (*pml4) & PHYSICAL_PAGE_MASK;
pgd_paddr = xen_m2p(pgd_paddr);
--
2.14.3
1:23 p.m.
New subject: [PATCH RFC 05/14] x86_64: clarify logical operator precedence
A fix has been queued for crash-7.2.1:
https://github.com/crash-utility/crash/commit/0f40db8fbac538ea448bbb2beb4...
Thanks,
Dave
----- Original Message -----
Signed-off-by: Oleksandr Natalenko <oleksandr(a)redhat.com>
---
x86_64.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/x86_64.c b/x86_64.c
index 2f9e6db..7b25e60 100644
--- a/x86_64.c
+++ b/x86_64.c
@@ -2086,7 +2086,7 @@ x86_64_kvtop(struct task_context *tc, ulong kvaddr,
physaddr_t *paddr, int verbo
}
start_vtop_with_pagetable:
- if (!(*pml4) & _PAGE_PRESENT)
+ if ((!(*pml4)) & _PAGE_PRESENT)
goto no_kpage;
pgd_paddr = (*pml4) & PHYSICAL_PAGE_MASK;
FILL_PGD(pgd_paddr, PHYSADDR, PAGESIZE());
@@ -2187,7 +2187,7 @@ x86_64_kvtop_xen_wpt(struct task_context *tc, ulong
kvaddr, physaddr_t *paddr, i
fprintf(fp, "PML4 DIRECTORY: %lx\n", vt->kernel_pgd[0]);
fprintf(fp, "PAGE DIRECTORY: %lx [machine]\n", *pml4);
}
- if (!(*pml4) & _PAGE_PRESENT)
+ if ((!(*pml4)) & _PAGE_PRESENT)
goto no_kpage;
pgd_paddr = (*pml4) & PHYSICAL_PAGE_MASK;
pgd_paddr = xen_m2p(pgd_paddr);
--
2.14.3
3:23 a.m.
New subject: [PATCH RFC 06/14] lkcd_v1: address of an array always evaluates to true
Signed-off-by: Oleksandr Natalenko <oleksandr(a)redhat.com>
---
lkcd_v1.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lkcd_v1.c b/lkcd_v1.c
index 5f891ae..499f146 100644
--- a/lkcd_v1.c
+++ b/lkcd_v1.c
@@ -249,7 +249,7 @@ dump_header_only:
lkcd_print(" dh_eip: %lx\n", dh->dh_eip);
lkcd_print(" dh_num_pages: %d\n", dh->dh_num_pages);
lkcd_print(" dh_panic_string: %s%s", dh->dh_panic_string,
- dh && dh->dh_panic_string &&
+ dh && *dh->dh_panic_string &&
strstr(dh->dh_panic_string, "\n") ? "" : "\n");
lkcd_print(" dh_time: %s\n",
strip_linefeeds(ctime(&(dh->dh_time.tv_sec))));
--
2.14.3
3:23 a.m.
New subject: [PATCH RFC 07/14] lkcd_v2_v3: address of an array always evaluates to true
Signed-off-by: Oleksandr Natalenko <oleksandr(a)redhat.com>
---
lkcd_v2_v3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lkcd_v2_v3.c b/lkcd_v2_v3.c
index 8635a7b..94a3918 100644
--- a/lkcd_v2_v3.c
+++ b/lkcd_v2_v3.c
@@ -307,7 +307,7 @@ dump_header_only:
lkcd_print(" dh_num_pages: ");
lkcd_print(BITS32() ? "%ld\n" : "%d\n", dh->dh_num_pages);
lkcd_print(" dh_panic_string: %s%s", dh->dh_panic_string,
- dh && dh->dh_panic_string &&
+ dh && *dh->dh_panic_string &&
strstr(dh->dh_panic_string, "\n") ? "" : "\n");
lkcd_print(" dh_time: %s\n",
strip_linefeeds(ctime(&(dh->dh_time.tv_sec))));
--
2.14.3
3:23 a.m.
New subject: [PATCH RFC 08/14] lkcd_v5: address of an array always evaluates to true
Signed-off-by: Oleksandr Natalenko <oleksandr(a)redhat.com>
---
lkcd_v5.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lkcd_v5.c b/lkcd_v5.c
index cb7634d..7214310 100644
--- a/lkcd_v5.c
+++ b/lkcd_v5.c
@@ -270,7 +270,7 @@ dump_header_only:
lkcd_print(" dh_num_pages: ");
lkcd_print(BITS32() ? "%ld\n" : "%d\n", dh->dh_num_pages);
lkcd_print(" dh_panic_string: %s%s", dh->dh_panic_string,
- dh && dh->dh_panic_string &&
+ dh && *dh->dh_panic_string &&
strstr(dh->dh_panic_string, "\n") ? "" : "\n");
lkcd_print(" dh_time: %s\n",
strip_linefeeds(ctime(&(dh->dh_time.tv_sec))));
--
2.14.3
3:23 a.m.
New subject: [PATCH RFC 09/14] lkcd_v7: address of an array always evaluates to true
Signed-off-by: Oleksandr Natalenko <oleksandr(a)redhat.com>
---
lkcd_v7.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lkcd_v7.c b/lkcd_v7.c
index 608e148..2122305 100644
--- a/lkcd_v7.c
+++ b/lkcd_v7.c
@@ -347,7 +347,7 @@ dump_header_only:
lkcd_print(" dh_num_pages: ");
lkcd_print(BITS32() ? "%ld\n" : "%d\n", dh->dh_num_pages);
lkcd_print(" dh_panic_string: %s%s", dh->dh_panic_string,
- dh && dh->dh_panic_string &&
+ dh && *dh->dh_panic_string &&
strstr(dh->dh_panic_string, "\n") ? "" : "\n");
lkcd_print(" dh_time: %s\n",
strip_linefeeds(ctime(&(dh->dh_time.tv_sec))));
--
2.14.3
3:23 a.m.
New subject: [PATCH RFC 10/14] lkcd_v8: address of an array always evaluates to true
Signed-off-by: Oleksandr Natalenko <oleksandr(a)redhat.com>
---
lkcd_v8.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lkcd_v8.c b/lkcd_v8.c
index 1322250..29966c8 100644
--- a/lkcd_v8.c
+++ b/lkcd_v8.c
@@ -542,7 +542,7 @@ dump_header_only:
lkcd_print(" dh_num_pages: ");
lkcd_print(BITS32() ? "%ld\n" : "%d\n", dh->dh_num_pages);
lkcd_print(" dh_panic_string: %s%s", dh->dh_panic_string,
- dh && dh->dh_panic_string &&
+ dh && *dh->dh_panic_string &&
strstr(dh->dh_panic_string, "\n") ? "" : "\n");
tv.tv_sec = dh->dh_time.tv_sec;
lkcd_print(" dh_time: %s\n",
--
2.14.3
3:23 a.m.
New subject: [PATCH RFC 11/14] qemu-load: remove unused function get_le64()
Signed-off-by: Oleksandr Natalenko <oleksandr(a)redhat.com>
---
qemu-load.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/qemu-load.c b/qemu-load.c
index 1c17fd9..9d8ffed 100644
--- a/qemu-load.c
+++ b/qemu-load.c
@@ -142,14 +142,6 @@ get_be64 (FILE *fp)
return ((uint64_t)a << 32) | b;
}
-static inline uint64_t
-get_le64 (FILE *fp)
-{
- uint32_t b = get_le32 (fp);
- uint32_t a = get_le32 (fp);
- return ((uint64_t)a << 32) | b;
-}
-
static inline void
get_qemu128 (FILE *fp, union qemu_uint128_t *result)
{
--
2.14.3
3:23 a.m.
New subject: [PATCH RFC 12/14] sadump: block_size is always non-negative
Signed-off-by: Oleksandr Natalenko <oleksandr(a)redhat.com>
---
sadump.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/sadump.c b/sadump.c
index 2ccfa82..6b912d4 100644
--- a/sadump.c
+++ b/sadump.c
@@ -157,9 +157,6 @@ read_dump_header(char *file)
}
restart:
- if (block_size < 0)
- return FALSE;
-
if (!read_device(sph, block_size, &offset)) {
error(INFO, "sadump: cannot read partition header\n");
goto err;
--
2.14.3
1:37 p.m.
New subject: [PATCH RFC 12/14] sadump: block_size is always non-negative
----- Original Message -----
> ... [ cut ] ...
>
> And I'm not sure whether this one should be fixed by just removing the
statement:
>
> --- a/sadump.c
> +++ b/sadump.c
> @@ -157,9 +157,6 @@ read_dump_header(char *file)
> }
>
> restart:
> - if (block_size < 0)
> - return FALSE;
> -
> if (!read_device(sph, block_size, &offset)) {
> error(INFO, "sadump: cannot read partition header\n");
> goto err;
>
>
> because farther down there is this:
>
> if (sh->block_size != block_size) {
> block_size = sh->block_size;
> offset = 0;
> goto restart;
> }
>
> I'll let the sadump maintainer decide how to deal with this one. He's on
> this list, but I've cc'd him to get his attention.
Thanks for telling me this, Dave.
Oleksandr, this looks good to me.
Thanks for your patch.
block_size is of type size_t and size_t is always defined as
some unsigned integer type in the C standards.
So, the condition on block_size is meaningless and can be removed.
Thanks for verifying the patch Daisuke -- queued for crash-7.2.1:
https://github.com/crash-utility/crash/commit/9339874f3764fe99a408aec1a81...
Dave
3:23 a.m.
New subject: [PATCH RFC 13/14] task: format string should be never empty
Signed-off-by: Oleksandr Natalenko <oleksandr(a)redhat.com>
---
task.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/task.c b/task.c
index 261ca31..fd8503f 100644
--- a/task.c
+++ b/task.c
@@ -790,10 +790,11 @@ refresh_fixed_task_table(void)
return;
if (DUMPFILE()) {
- fprintf(fp, (pc->flags & SILENT) || !(pc->flags & TTY) ?
- "" : "%splease wait... (gathering task table
data)",
- GDB_PATCHED() ? "" : "\n");
- fflush(fp);
+ if (!((pc->flags & SILENT) || !(pc->flags & TTY))) {
+ fprintf(fp, "%splease wait... (gathering task table data)",
+ GDB_PATCHED() ? "" : "\n");
+ fflush(fp);
+ }
if (!symbol_exists("panic_threads"))
tt->flags |= POPULATE_PANIC;
}
@@ -933,10 +934,11 @@ refresh_unlimited_task_table(void)
return;
if (DUMPFILE()) {
- fprintf(fp, (pc->flags & SILENT) || !(pc->flags & TTY) ?
- "" : "%splease wait... (gathering task table
data)",
- GDB_PATCHED() ? "" : "\n");
- fflush(fp);
+ if (!((pc->flags & SILENT) || !(pc->flags & TTY))) {
+ fprintf(fp, "%splease wait... (gathering task table data)",
+ GDB_PATCHED() ? "" : "\n");
+ fflush(fp);
+ }
if (!symbol_exists("panic_threads"))
tt->flags |= POPULATE_PANIC;
}
--
2.14.3
3:23 a.m.
New subject: [PATCH RFC 14/14] symbols: format string should be never empty
Signed-off-by: Oleksandr Natalenko <oleksandr(a)redhat.com>
---
symbols.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/symbols.c b/symbols.c
index 8a4c878..3e4c37a 100644
--- a/symbols.c
+++ b/symbols.c
@@ -12549,10 +12549,11 @@ patch_kernel_symbol(struct gnu_request *req)
"\nkernel relocated [%ldMB]: patching %ld gdb minimal_symbol values\n",
relocate_display, st->symcnt);
}
- fprintf(fp, (pc->flags & SILENT) || !(pc->flags & TTY) ?
"" :
- "\nplease wait... (patching %ld gdb minimal_symbol values) ",
- st->symcnt);
- fflush(fp);
+ if (!((pc->flags & SILENT) || !(pc->flags & TTY))) {
+ fprintf(fp, "\nplease wait... (patching %ld gdb minimal_symbol values) ",
+ st->symcnt);
+ fflush(fp);
+ }
req->count = 0;
req->length = 0;
req->last_sp = 0;
--
2.14.3
8:06 a.m.
----- Original Message -----
I've managed to instigate clang to build some pieces
of crash-utility code, and it showed unpleasant things
which I'd like to bring into your attention.
The patch bomb contains only minor nitpicks to check
whether I should continue with doing this kind of nasty job
further using static analysis tools, various compilers
and common sense.
I'll take a look at these when I get the chance, but I'm really
not particularly excited unless they are actual bugs.
Thanks,
Dave
Oleksandr Natalenko (14):
tools: fix non-literal NULL conversion
memory: do not compare unsigned expression with negative value
memory: use & for a bitwise operation
task: remove extra parentheses in equality comparison
x86_64: clarify logical operator precedence
lkcd_v1: address of an array always evaluates to true
lkcd_v2_v3: address of an array always evaluates to true
lkcd_v5: address of an array always evaluates to true
lkcd_v7: address of an array always evaluates to true
lkcd_v8: address of an array always evaluates to true
qemu-load: remove unused function get_le64()
sadump: block_size is always non-negative
task: format string should be never empty
symbols: format string should be never empty
lkcd_v1.c | 2 +-
lkcd_v2_v3.c | 2 +-
lkcd_v5.c | 2 +-
lkcd_v7.c | 2 +-
lkcd_v8.c | 2 +-
memory.c | 8 ++++----
qemu-load.c | 8 --------
sadump.c | 3 ---
symbols.c | 9 +++++----
task.c | 20 +++++++++++---------
tools.c | 2 +-
x86_64.c | 4 ++--
12 files changed, 28 insertions(+), 36 deletions(-)
--
2.14.3
2431
days inactive
2437
days old
devel@lists.crash-utility.osci.io
27 comments
3 participants
participants (3)
-
Dave Anderson -
Hatayama, Daisuke -
Oleksandr Natalenko