在 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 <lijiang@xxxxxxxxxx> >> --- >> 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. > 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@xxxxxxxxxxxxxxxxxxx >> http://lists.infradead.org/mailman/listinfo/kexec >> _______________________________________________ kexec mailing list kexec@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/kexec