[Qemu-devel] [PATCH] xen-hvm.c: Improve the return method for xen_hvm_init()

[Qemu-devel] [PATCH] xen-hvm.c: Improve the return method for xen_hvm_init()

[Chen Gang]

When failure occurs, it need use "return -1" instead of exit(1), so can
let upper caller has chance to print failure information, too, then user
can know the failure result more clearly.

xen_hvm_init() may also return -errno, which may let upper caller think
more (e.g. free some other related resources and try again), although at
present, all related upper callers still exit(1).

It is not a normal function which does not release related resources, if
return -1. So need give the related comments for it.

It passes common test under fedora 20 x86_64:

  "./configure --enable-xen && make -j4 && make check"
  execute result: "echo $? == 0".

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 xen-hvm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen-hvm.c b/xen-hvm.c
index 0d09940..35efec0 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -978,6 +978,7 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
     xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 0);
 }
 
+/* return value:  0 is OK, -errno is failure, -1 is critical issue -- exit(1) */
 int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
                  MemoryRegion **ram_memory)
 {
@@ -998,6 +999,7 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
     state->xenstore = xs_daemon_open();
     if (state->xenstore == NULL) {
         perror("xen: xenstore open");
+        xc_evtchn_close(state->xce_handle);
         g_free(state);
         return -errno;
     }
@@ -1069,7 +1071,7 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
     /* Initialize backend core & drivers */
     if (xen_be_init() != 0) {
         fprintf(stderr, "%s: xen backend core setup failed\n", __FUNCTION__);
-        exit(1);
+        return -1;
     }
     xen_be_register("console", &xen_console_ops);
     xen_be_register("vkbd", &xen_kbdmouse_ops);
-- 
1.9.3

Re: [Qemu-devel] [PATCH] xen-hvm.c: Improve the return method for xen_hvm_init()

[Chen Gang]

Oh, sorry, forgot Cc to qemu trivial.

Thanks.

On 9/3/14 0:22, Chen Gang wrote:
> When failure occurs, it need use "return -1" instead of exit(1), so can
> let upper caller has chance to print failure information, too, then user
> can know the failure result more clearly.
> 
> xen_hvm_init() may also return -errno, which may let upper caller think
> more (e.g. free some other related resources and try again), although at
> present, all related upper callers still exit(1).
> 
> It is not a normal function which does not release related resources, if
> return -1. So need give the related comments for it.
> 
> It passes common test under fedora 20 x86_64:
> 
>   "./configure --enable-xen && make -j4 && make check"
>   execute result: "echo $? == 0".
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  xen-hvm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/xen-hvm.c b/xen-hvm.c
> index 0d09940..35efec0 100644
> --- a/xen-hvm.c
> +++ b/xen-hvm.c
> @@ -978,6 +978,7 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
>      xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 0);
>  }
>  
> +/* return value:  0 is OK, -errno is failure, -1 is critical issue -- exit(1) */
>  int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
>                   MemoryRegion **ram_memory)
>  {
> @@ -998,6 +999,7 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
>      state->xenstore = xs_daemon_open();
>      if (state->xenstore == NULL) {
>          perror("xen: xenstore open");
> +        xc_evtchn_close(state->xce_handle);
>          g_free(state);
>          return -errno;
>      }
> @@ -1069,7 +1071,7 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
>      /* Initialize backend core & drivers */
>      if (xen_be_init() != 0) {
>          fprintf(stderr, "%s: xen backend core setup failed\n", __FUNCTION__);
> -        exit(1);
> +        return -1;
>      }
>      xen_be_register("console", &xen_console_ops);
>      xen_be_register("vkbd", &xen_kbdmouse_ops);
> 

-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [Qemu-devel] [PATCH] xen-hvm.c: Improve the return method for xen_hvm_init()

[Stefano Stabellini]

On Wed, 3 Sep 2014, Chen Gang wrote:
> Oh, sorry, forgot Cc to qemu trivial.
> 
> Thanks.
> On 9/3/14 0:22, Chen Gang wrote:
> > When failure occurs, it need use "return -1" instead of exit(1), so can
> > let upper caller has chance to print failure information, too, then user
> > can know the failure result more clearly.

good point


> > xen_hvm_init() may also return -errno, which may let upper caller think
> > more (e.g. free some other related resources and try again), although at
> > present, all related upper callers still exit(1).

I think we should return -1 in the other cases too


> > It is not a normal function which does not release related resources, if
> > return -1. So need give the related comments for it.
> > 
> > It passes common test under fedora 20 x86_64:
> > 
> >   "./configure --enable-xen && make -j4 && make check"
> >   execute result: "echo $? == 0".
> > 
> > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> > ---
> >  xen-hvm.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen-hvm.c b/xen-hvm.c
> > index 0d09940..35efec0 100644
> > --- a/xen-hvm.c
> > +++ b/xen-hvm.c
> > @@ -978,6 +978,7 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
> >      xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 0);
> >  }
> >  
> > +/* return value:  0 is OK, -errno is failure, -1 is critical issue -- exit(1) */
> >  int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
> >                   MemoryRegion **ram_memory)
> >  {
> > @@ -998,6 +999,7 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
> >      state->xenstore = xs_daemon_open();
> >      if (state->xenstore == NULL) {
> >          perror("xen: xenstore open");
> > +        xc_evtchn_close(state->xce_handle);
> >          g_free(state);
> >          return -errno;
> >      }

just return -1 and do the same in the above check (xce_handle ==
XC_HANDLER_INITIAL_VALUE)


> > @@ -1069,7 +1071,7 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
> >      /* Initialize backend core & drivers */
> >      if (xen_be_init() != 0) {
> >          fprintf(stderr, "%s: xen backend core setup failed\n", __FUNCTION__);
> > -        exit(1);
> > +        return -1;
> >      }
> >      xen_be_register("console", &xen_console_ops);
> >      xen_be_register("vkbd", &xen_kbdmouse_ops);
> > 
> 
> -- 
> Chen Gang
> 
> Open, share, and attitude like air, water, and life which God blessed
> 

Re: [Qemu-devel] [PATCH] xen-hvm.c: Improve the return method for xen_hvm_init()

[Chen Gang]

On 9/4/14 7:32, Stefano Stabellini wrote:
> On Wed, 3 Sep 2014, Chen Gang wrote:
>> On 9/3/14 0:22, Chen Gang wrote:
[...]
>>> xen_hvm_init() may also return -errno, which may let upper caller think
>>> more (e.g. free some other related resources and try again), although at
>>> present, all related upper callers still exit(1).
> 
> I think we should return -1 in the other cases too
> 

[...]
>>> @@ -998,6 +999,7 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
>>>      state->xenstore = xs_daemon_open();
>>>      if (state->xenstore == NULL) {
>>>          perror("xen: xenstore open");
>>> +        xc_evtchn_close(state->xce_handle);
>>>          g_free(state);
>>>          return -errno;
>>>      }
> 
> just return -1 and do the same in the above check (xce_handle ==
> XC_HANDLER_INITIAL_VALUE)
> 
> 

OK thanks, what you said sounds OK to me. I shall send patch v2 for it
within the 2 days (within 2014-09-05).

Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed