在 2019年08月23日 13:24, lijiang 写道:
> 在 2019年08月22日 16:51, Simon Horman 写道:
>> Hi Lianbo,
>>
>> I like where this patch is going but I would like to request a few changes.
>> Please see comments inline.
>>
>
> Thanks for your comment, Simon.
>
>> On Thu, Aug 15, 2019 at 11:37:55AM +0800, Lianbo Jiang wrote:
>>> Some code related to vmcore-dmesg.c is put into the util_lib, which
>>> is not very reasonable, so lets move it back and tidy up those code.
>>>
>>> In addition, that will also help to limit the size of vmcore-dmesg.txt.
>>>
>>> Signed-off-by: Lianbo Jiang <liji...@redhat.com>
>>> ---
>>> kexec/arch/arm64/kexec-arm64.c | 2 +-
>>> util_lib/elf_info.c | 73 ++++++++--------------------------
>>> util_lib/include/elf_info.h | 8 +++-
>>> vmcore-dmesg/vmcore-dmesg.c | 44 +++++++++++++++++---
>>> 4 files changed, 61 insertions(+), 66 deletions(-)
>>>
>>> diff --git a/kexec/arch/arm64/kexec-arm64.c b/kexec/arch/arm64/kexec-arm64.c
>>> index eb3a3a37307c..6ad3b0a134b3 100644
>>> --- a/kexec/arch/arm64/kexec-arm64.c
>>> +++ b/kexec/arch/arm64/kexec-arm64.c
>>> @@ -889,7 +889,7 @@ int get_phys_base_from_pt_load(unsigned long
>>> *phys_offset)
>>> return EFAILED;
>>> }
>>>
>>> - read_elf_kcore(fd);
>>> + read_elf(fd);
>>>
>>> for (i = 0; get_pt_load(i,
>>> &phys_start, NULL, &virt_start, NULL);
>>> diff --git a/util_lib/elf_info.c b/util_lib/elf_info.c
>>> index 90a3b21662e7..2f254e972721 100644
>>> --- a/util_lib/elf_info.c
>>> +++ b/util_lib/elf_info.c
>>> @@ -20,7 +20,6 @@
>>> /* The 32bit and 64bit note headers make it clear we don't care */
>>> typedef Elf32_Nhdr Elf_Nhdr;
>>>
>>> -static const char *fname;
>>> static Elf64_Ehdr ehdr;
>>> static Elf64_Phdr *phdr;
>>> static int num_pt_loads;
>>> @@ -120,8 +119,8 @@ void read_elf32(int fd)
>>>
>>> ret = pread(fd, &ehdr32, sizeof(ehdr32), 0);
>>> if (ret != sizeof(ehdr32)) {
>>> - fprintf(stderr, "Read of Elf header from %s failed: %s\n",
>>> - fname, strerror(errno));
>>> + fprintf(stderr, "Read of Elf header failed in %s: %s\n",
>>> + __func__, strerror(errno));
>>
>> I'm not sure of the merit of changing the loging output.
>
> The variable 'fname' is defined two twice, the first definition is in the
> vmcore-dmesg.c, and the
> second definition is in the elf_info.c. That is confused although it's a
> static type, because i do
> not see the place where the value of variable 'fname' is set in elf_info.c.
> So i guess that it should
> be a same variable within the vmcore-dmesg.c and also need to clean up.
>
BTW: i guess the original definition of 'fname' should look like this:
diff --git a/util_lib/elf_info.c b/util_lib/elf_info.c
index d9397ecd8626..5d0efaafab53 100644
--- a/util_lib/elf_info.c
+++ b/util_lib/elf_info.c
@@ -20,7 +20,7 @@
/* The 32bit and 64bit note headers make it clear we don't care */
typedef Elf32_Nhdr Elf_Nhdr;
-static const char *fname;
+const char *fname;
static Elf64_Ehdr ehdr;
static Elf64_Phdr *phdr;
static int num_pt_loads;
diff --git a/vmcore-dmesg/vmcore-dmesg.c b/vmcore-dmesg/vmcore-dmesg.c
index 7a386b380291..bebc348a657e 100644
--- a/vmcore-dmesg/vmcore-dmesg.c
+++ b/vmcore-dmesg/vmcore-dmesg.c
@@ -3,7 +3,7 @@
/* The 32bit and 64bit note headers make it clear we don't care */
typedef Elf32_Nhdr Elf_Nhdr;
-static const char *fname;
+extern const char *fname;
int main(int argc, char **argv)
{
>> And moreover I don't think it belongs in this patch
>> as it doesn't seem related to the other changes.
>>
>
> Good question. I will consider how to clean up. Probably, it should be a
> separate patch.
>
>>> exit(10);
>>> }
>>>
>>> @@ -193,8 +192,8 @@ void read_elf64(int fd)
>>>
>>> ret = pread(fd, &ehdr64, sizeof(ehdr64), 0);
>>> if (ret < 0 || (size_t)ret != sizeof(ehdr)) {
>>> - fprintf(stderr, "Read of Elf header from %s failed: %s\n",
>>> - fname, strerror(errno));
>>> + fprintf(stderr, "Read of Elf header failed in %s: %s\n",
>>> + __func__, strerror(errno));
>>> exit(10);
>>> }
>>>
>>> @@ -531,19 +530,7 @@ static int32_t read_file_s32(int fd, uint64_t addr)
>>> return read_file_u32(fd, addr);
>>> }
>>>
>>> -static void write_to_stdout(char *buf, unsigned int nr)
>>> -{
>>> - ssize_t ret;
>>> -
>>> - ret = write(STDOUT_FILENO, buf, nr);
>>> - if (ret != nr) {
>>> - fprintf(stderr, "Failed to write out the dmesg log buffer!:"
>>> - " %s\n", strerror(errno));
>>> - exit(54);
>>> - }
>>> -}
>>> -
>>> -static void dump_dmesg_legacy(int fd)
>>> +void dump_dmesg_legacy(int fd, handler_t handler)
>>> {
>>> uint64_t log_buf, log_buf_offset;
>>> unsigned log_end, logged_chars, log_end_wrapped;
>>> @@ -604,7 +591,7 @@ static void dump_dmesg_legacy(int fd)
>>> */
>>> logged_chars = log_end < log_buf_len ? log_end : log_buf_len;
>>>
>>> - write_to_stdout(buf + (log_buf_len - logged_chars), logged_chars);
>>> + handler(buf + (log_buf_len - logged_chars), logged_chars);
>>> }
>>>
>>> static inline uint16_t struct_val_u16(char *ptr, unsigned int offset)
>>> @@ -623,7 +610,7 @@ static inline uint64_t struct_val_u64(char *ptr,
>>> unsigned int offset)
>>> }
>>>
>>> /* Read headers of log records and dump accordingly */
>>> -static void dump_dmesg_structured(int fd)
>>> +void dump_dmesg_structured(int fd, handler_t handler)
>>> {
>>> #define OUT_BUF_SIZE 4096
>>> uint64_t log_buf, log_buf_offset, ts_nsec;
>>> @@ -733,7 +720,7 @@ static void dump_dmesg_structured(int fd)
>>> out_buf[len++] = c;
>>>
>>> if (len >= OUT_BUF_SIZE - 64) {
>>> - write_to_stdout(out_buf, len);
>>> + handler(out_buf, len);
>>> len = 0;
>>> }
>>> }
>>> @@ -753,25 +740,24 @@ static void dump_dmesg_structured(int fd)
>>> }
>>> free(buf);
>>> if (len)
>>> - write_to_stdout(out_buf, len);
>>> + handler(out_buf, len);
>>> }
>>>
>>> -static void dump_dmesg(int fd)
>>> +int check_log_first_idx_vaddr(void)
>>> {
>>> if (log_first_idx_vaddr)
>>> - dump_dmesg_structured(fd);
>>> - else
>>> - dump_dmesg_legacy(fd);
>>> + return 1;
>>> +
>>> + return 0;
>>> }
>>>
>>> -static int read_elf(int fd)
>>> +int read_elf(int fd)
>>> {
>>> int ret;
>>>
>>> ret = pread(fd, ehdr.e_ident, EI_NIDENT, 0);
>>> if (ret != EI_NIDENT) {
>>> - fprintf(stderr, "Read of e_ident from %s failed: %s\n",
>>> - fname, strerror(errno));
>>> + fprintf(stderr, "Read of e_ident failed: %s\n",
>>> strerror(errno));
>>> return 3;
>>> }
>>> if (memcmp(ehdr.e_ident, ELFMAG, SELFMAG) != 0) {
>>> @@ -808,40 +794,13 @@ static int read_elf(int fd)
>>> return 0;
>>> }
>>>
>>> -int read_elf_vmcore(int fd)
>>> -{
>>> - int ret;
>>> -
>>> - ret = read_elf(fd);
>>> - if (ret > 0) {
>>> - fprintf(stderr, "Unable to read ELF information"
>>> - " from vmcore\n");
>>> - return ret;
>>> - }
>>> -
>>> - dump_dmesg(fd);
>>> -
>>> - return 0;
>>> -}
>>> -
>>> -int read_elf_kcore(int fd)
>>> -{
>>> - int ret;
>>> -
>>> - ret = read_elf(fd);
>>> - if (ret != 0)
>>> - return ret;
>>> -
>>> - return 0;
>>> -}
>>
>> I think that removing read_elf_kcore is not related to the rest of this
>> patch and should be in a separate patch - it is a nice cleanup.
>>
> Good idea, i will split them into a separate patch.
>
>>> -
>>> int read_phys_offset_elf_kcore(int fd, unsigned long *phys_off)
>>> {
>>> int ret;
>>>
>>> *phys_off = UINT64_MAX;
>>>
>>> - ret = read_elf_kcore(fd);
>>> + ret = read_elf(fd);
>>> if (!ret) {
>>> /* If we have a valid 'PHYS_OFFSET' by now,
>>> * return it to the caller now.
>>> diff --git a/util_lib/include/elf_info.h b/util_lib/include/elf_info.h
>>> index 1a4debd2d4ba..8ee7d3e2763f 100644
>>> --- a/util_lib/include/elf_info.h
>>> +++ b/util_lib/include/elf_info.h
>>> @@ -23,13 +23,17 @@
>>> #include <inttypes.h>
>>> #include <ctype.h>
>>>
>>> +typedef void (*handler_t)(char *msg, unsigned int bytes);
>>
>> I would prefer it if we did not add new typedefs.
>>
>
> OK. I'm considering how to handle it. Any suggestions?
>
>>> +
>>> int get_pt_load(int idx,
>>> unsigned long long *phys_start,
>>> unsigned long long *phys_end,
>>> unsigned long long *virt_start,
>>> unsigned long long *virt_end);
>>> int read_phys_offset_elf_kcore(int fd, unsigned long *phys_off);
>>> -int read_elf_kcore(int fd);
>>> -int read_elf_vmcore(int fd);
>>> +int check_log_first_idx_vaddr(void);
>>> +void dump_dmesg_structured(int fd, handler_t handler);
>>> +void dump_dmesg_legacy(int fd, handler_t handler);
>>> +int read_elf(int fd);
>>>
>>> #endif /* ELF_INFO_H */
>>> diff --git a/vmcore-dmesg/vmcore-dmesg.c b/vmcore-dmesg/vmcore-dmesg.c
>>> index 7a386b380291..ff0d540c9130 100644
>>> --- a/vmcore-dmesg/vmcore-dmesg.c
>>> +++ b/vmcore-dmesg/vmcore-dmesg.c
>>> @@ -1,21 +1,53 @@
>>> #include <elf_info.h>
>>>
>>> -/* The 32bit and 64bit note headers make it clear we don't care */
>>> -typedef Elf32_Nhdr Elf_Nhdr;
>>> +static void write_to_stdout(char *buf, unsigned int nr)
>>> +{
>>> + ssize_t ret;
>>> +
>>> + ret = write(STDOUT_FILENO, buf, nr);
>>> + if (ret != nr) {
>>> + fprintf(stderr, "Failed to write out the dmesg log buffer!:"
>>> + " %s\n", strerror(errno));
>>> + exit(54);
>>> + }
>>> +}
>>> +
>>> +static void dump_dmesg(int fd, handler_t handler)
>>> +{
>>> + if (check_log_first_idx_vaddr())
>>> + dump_dmesg_structured(fd, handler);
>>> + else
>>> + dump_dmesg_legacy(fd, handler);
>>> +}
>>
>> I think that dump_dmesg() could stay in kexec-arm64.c,
>> it does not seem specific to vmcore handling.
>>
>
> The dump_dmesg() should be a common function, not only arm64 will use it, but
> x86 64 will also call it. It would be good to put the dump_dmesg() to a common
> place.
>
>>> -static const char *fname;
>>> +static int read_vmcore_dmesg(int fd)
>>> +{
>>> + int ret;
>>> +
>>> + ret = read_elf(fd);
>>> + if (ret > 0) {
>>> + fprintf(stderr, "Unable to read ELF information"
>>> + " from vmcore\n");
>>> + return ret;
>>> + }
>>> +
>>> + dump_dmesg(fd, write_to_stdout);
>>> +
>>> + return 0;
>>> +}
>>>
>>> int main(int argc, char **argv)
>>> {
>>> ssize_t ret;
>>> int fd;
>>> + const char *fname;
>>>
>>> if (argc != 2) {
>>> fprintf(stderr, "usage: %s <kernel core file>\n", argv[0]);
>>> return 1;
>>> }
>>> - fname = argv[1];
>>>
>>> + fname = argv[1];
>>> fd = open(fname, O_RDONLY);
>>> if (fd < 0) {
>>> fprintf(stderr, "Cannot open %s: %s\n",
>>> @@ -23,8 +55,8 @@ int main(int argc, char **argv)
>>> return 2;
>>> }
>>>
>>> - ret = read_elf_vmcore(fd);
>>> -
>>> + ret = read_vmcore_dmesg(fd);
>>> +
>>> close(fd);
>>>
>>> return ret;
>>> --
>>> 2.17.1
>>>
>>>
>>> _______________________________________________
>>> kexec mailing list
>>> kexec@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/kexec
>>>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
