Just like with the other write functions let's move the 32/64 bit elf
handling to a function to improve readability.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 dump/dump.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 78654b9c27..9ba0392e00 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -507,6 +507,21 @@ static void write_elf_loads(DumpState *s, Error **errp)
     }
 }
 
+static void write_elf_notes(DumpState *s, Error **errp)
+{
+    Error *local_err = NULL;
+
+    if (dump_is_64bit(s)) {
+        write_elf64_notes(fd_write_vmcore, s, &local_err);
+    } else {
+        write_elf32_notes(fd_write_vmcore, s, &local_err);
+    }
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+}
+
 /* write elf header, PT_NOTE and elf note to vmcore. */
 static void dump_begin(DumpState *s, Error **errp)
 {
@@ -570,13 +585,8 @@ static void dump_begin(DumpState *s, Error **errp)
         }
     }
 
-    if (dump_is_64bit(s)) {
-        /* write notes to vmcore */
-        write_elf64_notes(fd_write_vmcore, s, &local_err);
-    } else {
-        /* write notes to vmcore */
-        write_elf32_notes(fd_write_vmcore, s, &local_err);
-    }
+    /* write notes to vmcore */
+    write_elf_notes(s, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
-- 
2.32.0

Hi

On Tue, Mar 1, 2022 at 6:22 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>
> Just like with the other write functions let's move the 32/64 bit elf
> handling to a function to improve readability.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  dump/dump.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 78654b9c27..9ba0392e00 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -507,6 +507,21 @@ static void write_elf_loads(DumpState *s, Error **errp)
>      }
>  }
>
> +static void write_elf_notes(DumpState *s, Error **errp)
> +{
> +    Error *local_err = NULL;
> +
> +    if (dump_is_64bit(s)) {
> +        write_elf64_notes(fd_write_vmcore, s, &local_err);
> +    } else {
> +        write_elf32_notes(fd_write_vmcore, s, &local_err);
> +    }
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }

Please use "modern"-style ERRP_GUARD(), and indicate failure with a
bool (see include/qapi/error.h)

(perhaps this should be preliminary to this series)

> +}
> +
>  /* write elf header, PT_NOTE and elf note to vmcore. */
>  static void dump_begin(DumpState *s, Error **errp)
>  {
> @@ -570,13 +585,8 @@ static void dump_begin(DumpState *s, Error **errp)
>          }
>      }
>
> -    if (dump_is_64bit(s)) {
> -        /* write notes to vmcore */
> -        write_elf64_notes(fd_write_vmcore, s, &local_err);
> -    } else {
> -        /* write notes to vmcore */
> -        write_elf32_notes(fd_write_vmcore, s, &local_err);
> -    }
> +    /* write notes to vmcore */
> +    write_elf_notes(s, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> --
> 2.32.0
>

On 3/2/22 11:30, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Mar 1, 2022 at 6:22 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>>
>> Just like with the other write functions let's move the 32/64 bit elf
>> handling to a function to improve readability.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   dump/dump.c | 24 +++++++++++++++++-------
>>   1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/dump/dump.c b/dump/dump.c
>> index 78654b9c27..9ba0392e00 100644
>> --- a/dump/dump.c
>> +++ b/dump/dump.c
>> @@ -507,6 +507,21 @@ static void write_elf_loads(DumpState *s, Error **errp)
>>       }
>>   }
>>
>> +static void write_elf_notes(DumpState *s, Error **errp)
>> +{
>> +    Error *local_err = NULL;
>> +
>> +    if (dump_is_64bit(s)) {
>> +        write_elf64_notes(fd_write_vmcore, s, &local_err);
>> +    } else {
>> +        write_elf32_notes(fd_write_vmcore, s, &local_err);
>> +    }
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
> 
> Please use "modern"-style ERRP_GUARD(), and indicate failure with a
> bool (see include/qapi/error.h)

Didn't know that's a thing, I'll have a look

> 
> (perhaps this should be preliminary to this series)

So you want me to change all the local_error + error_propagate()s in 
this file?

> 
>> +}
>> +
>>   /* write elf header, PT_NOTE and elf note to vmcore. */
>>   static void dump_begin(DumpState *s, Error **errp)
>>   {
>> @@ -570,13 +585,8 @@ static void dump_begin(DumpState *s, Error **errp)
>>           }
>>       }
>>
>> -    if (dump_is_64bit(s)) {
>> -        /* write notes to vmcore */
>> -        write_elf64_notes(fd_write_vmcore, s, &local_err);
>> -    } else {
>> -        /* write notes to vmcore */
>> -        write_elf32_notes(fd_write_vmcore, s, &local_err);
>> -    }
>> +    /* write notes to vmcore */
>> +    write_elf_notes(s, &local_err);
>>       if (local_err) {
>>           error_propagate(errp, local_err);
>>           return;
>> --
>> 2.32.0
>>
> 

Hi

On Wed, Mar 2, 2022 at 6:02 PM Janosch Frank <frankja@linux.ibm.com> wrote:

> On 3/2/22 11:30, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Mar 1, 2022 at 6:22 PM Janosch Frank <frankja@linux.ibm.com>
> wrote:
> >>
> >> Just like with the other write functions let's move the 32/64 bit elf
> >> handling to a function to improve readability.
> >>
> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >> ---
> >>   dump/dump.c | 24 +++++++++++++++++-------
> >>   1 file changed, 17 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/dump/dump.c b/dump/dump.c
> >> index 78654b9c27..9ba0392e00 100644
> >> --- a/dump/dump.c
> >> +++ b/dump/dump.c
> >> @@ -507,6 +507,21 @@ static void write_elf_loads(DumpState *s, Error
> **errp)
> >>       }
> >>   }
> >>
> >> +static void write_elf_notes(DumpState *s, Error **errp)
> >> +{
> >> +    Error *local_err = NULL;
> >> +
> >> +    if (dump_is_64bit(s)) {
> >> +        write_elf64_notes(fd_write_vmcore, s, &local_err);
> >> +    } else {
> >> +        write_elf32_notes(fd_write_vmcore, s, &local_err);
> >> +    }
> >> +    if (local_err) {
> >> +        error_propagate(errp, local_err);
> >> +        return;
> >> +    }
> >
> > Please use "modern"-style ERRP_GUARD(), and indicate failure with a
> > bool (see include/qapi/error.h)
>
> Didn't know that's a thing, I'll have a look
>
> >
> > (perhaps this should be preliminary to this series)
>
> So you want me to change all the local_error + error_propagate()s in
> this file?
>
>
It's not mandatory, but if you do that would be nice. Otherwise, try to
update the code you touch.

thanks



> >
> >> +}
> >> +
> >>   /* write elf header, PT_NOTE and elf note to vmcore. */
> >>   static void dump_begin(DumpState *s, Error **errp)
> >>   {
> >> @@ -570,13 +585,8 @@ static void dump_begin(DumpState *s, Error **errp)
> >>           }
> >>       }
> >>
> >> -    if (dump_is_64bit(s)) {
> >> -        /* write notes to vmcore */
> >> -        write_elf64_notes(fd_write_vmcore, s, &local_err);
> >> -    } else {
> >> -        /* write notes to vmcore */
> >> -        write_elf32_notes(fd_write_vmcore, s, &local_err);
> >> -    }
> >> +    /* write notes to vmcore */
> >> +    write_elf_notes(s, &local_err);
> >>       if (local_err) {
> >>           error_propagate(errp, local_err);
> >>           return;
> >> --
> >> 2.32.0
> >>
> >
>
>

-- 
Marc-André Lureau