Message ID | 5405EED9.2090701@gmail.com |
---|---|
State | New |
Headers | show |
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); >
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 >
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.
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);
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(-)