Re: [PATCH 1/2] cleanup: move it back from util_lib/elf_info.c
在 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
>>> ---
>>> 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,
>>> _start, NULL, _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, , 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, , 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;
>>>
Re: [PATCH 1/2] cleanup: move it back from util_lib/elf_info.c
在 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
>> ---
>> 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,
>> _start, NULL, _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, , 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, , 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_SIZE4096
>> 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;
>> }
>> }
>>
Re: [PATCH 1/2] cleanup: move it back from util_lib/elf_info.c
Hi Lianbo,
I like where this patch is going but I would like to request a few changes.
Please see comments inline.
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
> ---
> 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,
> _start, NULL, _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, , 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.
And moreover I don't think it belongs in this patch
as it doesn't seem related to the other changes.
> exit(10);
> }
>
> @@ -193,8 +192,8 @@ void read_elf64(int fd)
>
> ret = pread(fd, , 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) {
>
[PATCH 1/2] cleanup: move it back from util_lib/elf_info.c
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
---
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,
_start, NULL, _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, , 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));
exit(10);
}
@@ -193,8 +192,8 @@ void read_elf64(int fd)
ret = pread(fd, , 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) {
-
