[PATCH v3 13/13] xen-hvm: Mark inappropriate error handling FIXME

[PATCH v3 13/13] xen-hvm: Mark inappropriate error handling FIXME

[Markus Armbruster]

Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: xen-devel@lists.xensource.com
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 xen-hvm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen-hvm.c b/xen-hvm.c
index 3d78a0c..2a93390 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -240,6 +240,7 @@ static void xen_ram_init(PCMachineState *pcms,
 
 void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
 {
+    /* FIXME caller ram_block_add() wants error_setg() on failure */
     unsigned long nr_pfn;
     xen_pfn_t *pfn_list;
     int i;
@@ -1192,6 +1193,12 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
 int xen_hvm_init(PCMachineState *pcms,
                  MemoryRegion **ram_memory)
 {
+    /*
+     * FIXME Returns -1 without cleaning up on some errors (harmless
+     * as long as the caller exit()s on error), dies with hw_error()
+     * on others.  hw_error() isn't approprate here.  Should probably
+     * simply exit() on all errors.
+     */
     int i, rc;
     xen_pfn_t ioreq_pfn;
     xen_pfn_t bufioreq_pfn;
-- 
2.4.3

Re: [Xen-devel] [PATCH v3 13/13] xen-hvm: Mark inappropriate error handling FIXME

[Stefano Stabellini]

On Thu, 17 Dec 2015, Markus Armbruster wrote:
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: xen-devel@lists.xensource.com
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  xen-hvm.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen-hvm.c b/xen-hvm.c
> index 3d78a0c..2a93390 100644
> --- a/xen-hvm.c
> +++ b/xen-hvm.c
> @@ -240,6 +240,7 @@ static void xen_ram_init(PCMachineState *pcms,
>  
>  void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
>  {
> +    /* FIXME caller ram_block_add() wants error_setg() on failure */
>      unsigned long nr_pfn;
>      xen_pfn_t *pfn_list;
>      int i;
> @@ -1192,6 +1193,12 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
>  int xen_hvm_init(PCMachineState *pcms,
>                   MemoryRegion **ram_memory)
>  {
> +    /*
> +     * FIXME Returns -1 without cleaning up on some errors (harmless
> +     * as long as the caller exit()s on error), dies with hw_error()

Could you please stop the comment here and just replace hw_error() with
either return -1 or exit(1) within xen_hvm_init?


> +     * on others.  hw_error() isn't approprate here.  Should probably
> +     * simply exit() on all errors.
> +     */
>      int i, rc;
>      xen_pfn_t ioreq_pfn;
>      xen_pfn_t bufioreq_pfn;
> -- 
> 2.4.3
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [Xen-devel] [PATCH v3 13/13] xen-hvm: Mark inappropriate error handling FIXME

[Markus Armbruster]

Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:

> On Thu, 17 Dec 2015, Markus Armbruster wrote:
>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Cc: xen-devel@lists.xensource.com
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  xen-hvm.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>> 
>> diff --git a/xen-hvm.c b/xen-hvm.c
>> index 3d78a0c..2a93390 100644
>> --- a/xen-hvm.c
>> +++ b/xen-hvm.c
>> @@ -240,6 +240,7 @@ static void xen_ram_init(PCMachineState *pcms,
>>  
>>  void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
>>  {
>> +    /* FIXME caller ram_block_add() wants error_setg() on failure */
>>      unsigned long nr_pfn;
>>      xen_pfn_t *pfn_list;
>>      int i;
>> @@ -1192,6 +1193,12 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
>>  int xen_hvm_init(PCMachineState *pcms,
>>                   MemoryRegion **ram_memory)
>>  {
>> +    /*
>> +     * FIXME Returns -1 without cleaning up on some errors (harmless
>> +     * as long as the caller exit()s on error), dies with hw_error()
>
> Could you please stop the comment here and just replace hw_error() with
> either return -1 or exit(1) within xen_hvm_init?

If compile-testing is okay, I can either

(1) replace hw_error() by return -1, or

(2) replace both hw_error() and return -1 by exit(1), and make
xen_hvm_init() return void.

Your pick?

>
>> +     * on others.  hw_error() isn't approprate here.  Should probably
>> +     * simply exit() on all errors.
>> +     */
>>      int i, rc;
>>      xen_pfn_t ioreq_pfn;
>>      xen_pfn_t bufioreq_pfn;

Re: [Xen-devel] [PATCH v3 13/13] xen-hvm: Mark inappropriate error handling FIXME

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
>
>> On Thu, 17 Dec 2015, Markus Armbruster wrote:
>>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>> Cc: xen-devel@lists.xensource.com
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  xen-hvm.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>> 
>>> diff --git a/xen-hvm.c b/xen-hvm.c
>>> index 3d78a0c..2a93390 100644
>>> --- a/xen-hvm.c
>>> +++ b/xen-hvm.c
>>> @@ -240,6 +240,7 @@ static void xen_ram_init(PCMachineState *pcms,
>>>  
>>>  void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
>>>  {
>>> +    /* FIXME caller ram_block_add() wants error_setg() on failure */
>>>      unsigned long nr_pfn;
>>>      xen_pfn_t *pfn_list;
>>>      int i;
>>> @@ -1192,6 +1193,12 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
>>>  int xen_hvm_init(PCMachineState *pcms,
>>>                   MemoryRegion **ram_memory)
>>>  {
>>> +    /*
>>> +     * FIXME Returns -1 without cleaning up on some errors (harmless
>>> +     * as long as the caller exit()s on error), dies with hw_error()
>>
>> Could you please stop the comment here and just replace hw_error() with
>> either return -1 or exit(1) within xen_hvm_init?
>
> If compile-testing is okay, I can either
>
> (1) replace hw_error() by return -1, or
>
> (2) replace both hw_error() and return -1 by exit(1), and make
> xen_hvm_init() return void.
>
> Your pick?

If you don't mind, I'll do (2) as a follow-up patch.  (2) because I like
it better, and follow-up patch because I'd prefer not to delay my error
pull request any longer.

Re: [Xen-devel] [PATCH v3 13/13] xen-hvm: Mark inappropriate error handling FIXME

[Stefano Stabellini]

On Wed, 13 Jan 2016, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
> >
> >> On Thu, 17 Dec 2015, Markus Armbruster wrote:
> >>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >>> Cc: xen-devel@lists.xensource.com
> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >>> ---
> >>>  xen-hvm.c | 7 +++++++
> >>>  1 file changed, 7 insertions(+)
> >>> 
> >>> diff --git a/xen-hvm.c b/xen-hvm.c
> >>> index 3d78a0c..2a93390 100644
> >>> --- a/xen-hvm.c
> >>> +++ b/xen-hvm.c
> >>> @@ -240,6 +240,7 @@ static void xen_ram_init(PCMachineState *pcms,
> >>>  
> >>>  void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
> >>>  {
> >>> +    /* FIXME caller ram_block_add() wants error_setg() on failure */
> >>>      unsigned long nr_pfn;
> >>>      xen_pfn_t *pfn_list;
> >>>      int i;
> >>> @@ -1192,6 +1193,12 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
> >>>  int xen_hvm_init(PCMachineState *pcms,
> >>>                   MemoryRegion **ram_memory)
> >>>  {
> >>> +    /*
> >>> +     * FIXME Returns -1 without cleaning up on some errors (harmless
> >>> +     * as long as the caller exit()s on error), dies with hw_error()
> >>
> >> Could you please stop the comment here and just replace hw_error() with
> >> either return -1 or exit(1) within xen_hvm_init?
> >
> > If compile-testing is okay, I can either
> >
> > (1) replace hw_error() by return -1, or
> >
> > (2) replace both hw_error() and return -1 by exit(1), and make
> > xen_hvm_init() return void.
> >
> > Your pick?
> 
> If you don't mind, I'll do (2) as a follow-up patch.  (2) because I like
> it better, and follow-up patch because I'd prefer not to delay my error
> pull request any longer.

That's fine. Sorry, I lost your previous email.