* [PATCH v3 2/6] libxl: Fix libxl_set_memory_target return value
2015-12-01 11:53 [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach George Dunlap
@ 2015-12-01 11:53 ` George Dunlap
2015-12-08 17:19 ` Ian Campbell
2015-12-01 11:53 ` [PATCH v3 3/6] xl: Make set_memory_target return an error code on failure George Dunlap
` (5 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: George Dunlap @ 2015-12-01 11:53 UTC (permalink / raw)
To: xen-devel
Cc: George Dunlap, Ian Jackson, Wei Liu, George Dunlap, Ian Campbell
libxl_set_memory_target seems to have the following return values:
* 1 on failure, if the failure happens because of a xenstore error *or* invalid target
* -1 if the setmaxmem hypercall
* -errno if the set_pod_target hypercall target fails
* 1 on success (!)
Make it consistenstly return ERROR_FAIL, unless the parameters were
invalid.
To make this more robust, use 'lrc' for return values to functions
whose return values are a different error space (like
xc_domain_setmaxmem and xc_domain_set_pod_target), or where a failure
means retry, rather than fail the whole function
(libxl__fill_dom0_memory_info), to reduce the risk that future code
shuffles will accidentally clobber the return value again.
Also remove the final call to xc_domain_getinfolist. There's no
obvious reason for this call -- all it seems to be doing is checking
to see if the domain exists; but if it doesn't exist, it will have
already failed by this point.
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/libxl.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 814d056..f8a0642 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4722,7 +4722,7 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
int32_t target_memkb, int relative, int enforce)
{
GC_INIT(ctx);
- int rc = 1, abort_transaction = 0;
+ int rc = ERROR_FAIL, abort_transaction = 0, lrc;
uint64_t memorykb;
uint32_t videoram = 0;
uint32_t current_target_memkb = 0, new_target_memkb = 0;
@@ -4750,9 +4750,9 @@ retry_transaction:
if (!target && !domid) {
if (!xs_transaction_end(ctx->xsh, t, 1))
goto out_no_transaction;
- rc = libxl__fill_dom0_memory_info(gc, ¤t_target_memkb,
+ lrc = libxl__fill_dom0_memory_info(gc, ¤t_target_memkb,
¤t_max_memkb);
- if (rc < 0)
+ if (lrc < 0)
goto out_no_transaction;
goto retry_transaction;
} else if (!target) {
@@ -4800,6 +4800,7 @@ retry_transaction:
"memory_dynamic_max must be less than or equal to"
" memory_static_max\n");
abort_transaction = 1;
+ rc = ERROR_INVAL;
goto out;
}
@@ -4807,43 +4808,39 @@ retry_transaction:
LOG(ERROR, "new target %d for dom0 is below the minimum threshold",
new_target_memkb);
abort_transaction = 1;
+ rc = ERROR_INVAL;
goto out;
}
if (enforce) {
memorykb = new_target_memkb + videoram;
- rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
+ lrc = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
LIBXL_MAXMEM_CONSTANT);
- if (rc != 0) {
+ if (lrc != 0) {
LOGE(ERROR,
"xc_domain_setmaxmem domid=%u memkb=%"PRIu64" failed ""rc=%d\n",
domid,
memorykb + LIBXL_MAXMEM_CONSTANT,
- rc);
+ lrc);
abort_transaction = 1;
goto out;
}
}
- rc = xc_domain_set_pod_target(ctx->xch, domid,
+ lrc = xc_domain_set_pod_target(ctx->xch, domid,
(new_target_memkb + LIBXL_MAXMEM_CONSTANT) / 4, NULL, NULL, NULL);
- if (rc != 0) {
+ if (lrc != 0) {
LOGE(ERROR,
"xc_domain_set_pod_target domid=%d, memkb=%d ""failed rc=%d\n",
domid,
new_target_memkb / 4,
- rc);
+ lrc);
abort_transaction = 1;
goto out;
}
libxl__xs_write(gc, t, GCSPRINTF("%s/memory/target",
dompath), "%"PRIu32, new_target_memkb);
- rc = xc_domain_getinfolist(ctx->xch, domid, 1, &info);
- if (rc != 1 || info.domain != domid) {
- abort_transaction = 1;
- goto out;
- }
libxl_dominfo_init(&ptr);
xcinfo2xlinfo(ctx, &info, &ptr);
@@ -4852,6 +4849,8 @@ retry_transaction:
"%"PRIu32, new_target_memkb / 1024);
libxl_dominfo_dispose(&ptr);
+ rc = 0;
+
out:
if (!xs_transaction_end(ctx->xsh, t, abort_transaction)
&& !abort_transaction)
--
2.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/6] libxl: Fix libxl_set_memory_target return value
2015-12-01 11:53 ` [PATCH v3 2/6] libxl: Fix libxl_set_memory_target return value George Dunlap
@ 2015-12-08 17:19 ` Ian Campbell
2015-12-08 17:25 ` George Dunlap
0 siblings, 1 reply; 18+ messages in thread
From: Ian Campbell @ 2015-12-08 17:19 UTC (permalink / raw)
To: George Dunlap, xen-devel; +Cc: Ian Jackson, Wei Liu, George Dunlap
On Tue, 2015-12-01 at 11:53 +0000, George Dunlap wrote:
> libxl_set_memory_target seems to have the following return values:
>
> * 1 on failure, if the failure happens because of a xenstore error *or*
> invalid target
>
> * -1 if the setmaxmem hypercall
>
> * -errno if the set_pod_target hypercall target fails
>
> * 1 on success (!)
>
> Make it consistenstly return ERROR_FAIL, unless the parameters were
"consistently"
> invalid.
>
> To make this more robust, use 'lrc' for return values to functions
tools/libxl/CODING_STYLE recommends "r" for such variables (return values
of syscalls or libxc calls).
> whose return values are a different error space (like
> xc_domain_setmaxmem and xc_domain_set_pod_target), or where a failure
> means retry, rather than fail the whole function
> (libxl__fill_dom0_memory_info), to reduce the risk that future code
> shuffles will accidentally clobber the return value again.
>
> Also remove the final call to xc_domain_getinfolist. There's no
> obvious reason for this call -- all it seems to be doing is checking
> to see if the domain exists; but if it doesn't exist, it will have
> already failed by this point.
If we aren't sure what it is for then I'd rather remove it in an
independent change, just in case.
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
> tools/libxl/libxl.c | 27 +++++++++++++--------------
> 1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 814d056..f8a0642 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4722,7 +4722,7 @@ int libxl_set_memory_target(libxl_ctx *ctx,
> uint32_t domid,
> int32_t target_memkb, int relative, int enforce)
> {
> GC_INIT(ctx);
> - int rc = 1, abort_transaction = 0;
> + int rc = ERROR_FAIL, abort_transaction = 0, lrc;
CODING_STYLE asks that rc not be initialised on declaration but set on the
failure paths (it allows a single line if () { rc = ; goto ... } to
mitigate the verbosity of this somewhat).
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/6] libxl: Fix libxl_set_memory_target return value
2015-12-08 17:19 ` Ian Campbell
@ 2015-12-08 17:25 ` George Dunlap
0 siblings, 0 replies; 18+ messages in thread
From: George Dunlap @ 2015-12-08 17:25 UTC (permalink / raw)
To: Ian Campbell, George Dunlap, xen-devel; +Cc: Ian Jackson, Wei Liu
On 08/12/15 17:19, Ian Campbell wrote:
> On Tue, 2015-12-01 at 11:53 +0000, George Dunlap wrote:
>> libxl_set_memory_target seems to have the following return values:
>>
>> * 1 on failure, if the failure happens because of a xenstore error *or*
>> invalid target
>>
>> * -1 if the setmaxmem hypercall
>>
>> * -errno if the set_pod_target hypercall target fails
>>
>> * 1 on success (!)
>>
>> Make it consistenstly return ERROR_FAIL, unless the parameters were
>
> "consistently"
>
>> invalid.
>>
>> To make this more robust, use 'lrc' for return values to functions
>
> tools/libxl/CODING_STYLE recommends "r" for such variables (return values
> of syscalls or libxc calls).
>
>> whose return values are a different error space (like
>> xc_domain_setmaxmem and xc_domain_set_pod_target), or where a failure
>> means retry, rather than fail the whole function
>> (libxl__fill_dom0_memory_info), to reduce the risk that future code
>> shuffles will accidentally clobber the return value again.
>>
>> Also remove the final call to xc_domain_getinfolist. There's no
>> obvious reason for this call -- all it seems to be doing is checking
>> to see if the domain exists; but if it doesn't exist, it will have
>> already failed by this point.
>
> If we aren't sure what it is for then I'd rather remove it in an
> independent change, just in case.
>
>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>> ---
>> CC: Ian Campbell <ian.campbell@citrix.com>
>> CC: Ian Jackson <ian.jackson@citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> ---
>> tools/libxl/libxl.c | 27 +++++++++++++--------------
>> 1 file changed, 13 insertions(+), 14 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 814d056..f8a0642 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -4722,7 +4722,7 @@ int libxl_set_memory_target(libxl_ctx *ctx,
>> uint32_t domid,
>> int32_t target_memkb, int relative, int enforce)
>> {
>> GC_INIT(ctx);
>> - int rc = 1, abort_transaction = 0;
>> + int rc = ERROR_FAIL, abort_transaction = 0, lrc;
>
> CODING_STYLE asks that rc not be initialised on declaration but set on the
> failure paths (it allows a single line if () { rc = ; goto ... } to
> mitigate the verbosity of this somewhat).
Ack to all of the above.
-George
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 3/6] xl: Make set_memory_target return an error code on failure
2015-12-01 11:53 [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach George Dunlap
2015-12-01 11:53 ` [PATCH v3 2/6] libxl: Fix libxl_set_memory_target return value George Dunlap
@ 2015-12-01 11:53 ` George Dunlap
2015-12-08 17:28 ` Ian Campbell
2015-12-01 11:53 ` [PATCH v3 4/6] xl: Return an error on failed cd-insert George Dunlap
` (4 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: George Dunlap @ 2015-12-01 11:53 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Ian Jackson, Wei Liu, Ian Campbell
Bring set_memory_target into line with set_memory_max (which does
return an error code.
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Konrad Wilk <konrad.wilk@oracle.com>
---
tools/libxl/xl_cmdimpl.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 2ba2393..4455d73 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3297,9 +3297,10 @@ int main_memmax(int argc, char **argv)
return 0;
}
-static void set_memory_target(uint32_t domid, const char *mem)
+static int set_memory_target(uint32_t domid, const char *mem)
{
- long long int memorykb;
+ int64_t memorykb;
+ int rc;
memorykb = parse_mem_size_kb(mem);
if (memorykb == -1) {
@@ -3307,7 +3308,9 @@ static void set_memory_target(uint32_t domid, const char *mem)
exit(3);
}
- libxl_set_memory_target(ctx, domid, memorykb, 0, /* enforce */ 1);
+ rc = libxl_set_memory_target(ctx, domid, memorykb, 0, /* enforce */ 1);
+
+ return rc;
}
int main_memset(int argc, char **argv)
@@ -3315,6 +3318,7 @@ int main_memset(int argc, char **argv)
uint32_t domid;
int opt = 0;
const char *mem;
+ int rc;
SWITCH_FOREACH_OPT(opt, "", NULL, "mem-set", 2) {
/* No options */
@@ -3323,7 +3327,12 @@ int main_memset(int argc, char **argv)
domid = find_domain(argv[optind]);
mem = argv[optind + 1];
- set_memory_target(domid, mem);
+ rc = set_memory_target(domid, mem);
+ if (rc) {
+ fprintf(stderr, "cannot set domid %d dynamic max memory to : %s\n", domid, mem);
+ return 1;
+ }
+
return 0;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3 3/6] xl: Make set_memory_target return an error code on failure
2015-12-01 11:53 ` [PATCH v3 3/6] xl: Make set_memory_target return an error code on failure George Dunlap
@ 2015-12-08 17:28 ` Ian Campbell
0 siblings, 0 replies; 18+ messages in thread
From: Ian Campbell @ 2015-12-08 17:28 UTC (permalink / raw)
To: George Dunlap, xen-devel; +Cc: Ian Jackson, Wei Liu
On Tue, 2015-12-01 at 11:53 +0000, George Dunlap wrote:
> Bring set_memory_target into line with set_memory_max (which does
> return an error code.
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Konrad Wilk <konrad.wilk@oracle.com>
> ---
> tools/libxl/xl_cmdimpl.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 2ba2393..4455d73 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3297,9 +3297,10 @@ int main_memmax(int argc, char **argv)
> return 0;
> }
>
> -static void set_memory_target(uint32_t domid, const char *mem)
> +static int set_memory_target(uint32_t domid, const char *mem)
> {
> - long long int memorykb;
> + int64_t memorykb;
The switch from long long to int64_t here is just incidental, right?
It did cause me to notice that both libxl_set_memory_target
and libxl_domain_setmaxmem take a 32bit (inconsistently signed vs unsigned)
argument for the memkb, so apart from the loss of range vs
parse_mem_size_kb you also can't set the target as high as you can set the
maximum. Nice.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 4/6] xl: Return an error on failed cd-insert
2015-12-01 11:53 [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach George Dunlap
2015-12-01 11:53 ` [PATCH v3 2/6] libxl: Fix libxl_set_memory_target return value George Dunlap
2015-12-01 11:53 ` [PATCH v3 3/6] xl: Make set_memory_target return an error code on failure George Dunlap
@ 2015-12-01 11:53 ` George Dunlap
2015-12-08 17:29 ` Ian Campbell
2015-12-01 11:53 ` [PATCH v3 5/6] xl: Return error codes for pci* commands George Dunlap
` (3 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: George Dunlap @ 2015-12-01 11:53 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Ian Jackson, Wei Liu, Ian Campbell
This makes xl more useful in scripts.
The strange thing about this is that the internal cd_insert function
*already* returned something appropriate, and cd-eject was using it,
but cd-insert wasnt.
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/xl_cmdimpl.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 4455d73..72ece2e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3405,8 +3405,7 @@ int main_cd_insert(int argc, char **argv)
virtdev = argv[optind + 1];
file = argv[optind + 2];
- cd_insert(domid, virtdev, file);
- return 0;
+ return cd_insert(domid, virtdev, file);
}
int main_console(int argc, char **argv)
--
2.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3 4/6] xl: Return an error on failed cd-insert
2015-12-01 11:53 ` [PATCH v3 4/6] xl: Return an error on failed cd-insert George Dunlap
@ 2015-12-08 17:29 ` Ian Campbell
0 siblings, 0 replies; 18+ messages in thread
From: Ian Campbell @ 2015-12-08 17:29 UTC (permalink / raw)
To: George Dunlap, xen-devel; +Cc: Ian Jackson, Wei Liu
On Tue, 2015-12-01 at 11:53 +0000, George Dunlap wrote:
> This makes xl more useful in scripts.
>
> The strange thing about this is that the internal cd_insert function
> *already* returned something appropriate, and cd-eject was using it,
> but cd-insert wasnt.
Missing an apostrophe in wasnt.
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Acked-by: Ian Campbell <Ian.campbell@citrix.com>
> ---
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
> tools/libxl/xl_cmdimpl.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 4455d73..72ece2e 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3405,8 +3405,7 @@ int main_cd_insert(int argc, char **argv)
> virtdev = argv[optind + 1];
> file = argv[optind + 2];
>
> - cd_insert(domid, virtdev, file);
> - return 0;
> + return cd_insert(domid, virtdev, file);
> }
>
> int main_console(int argc, char **argv)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 5/6] xl: Return error codes for pci* commands
2015-12-01 11:53 [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach George Dunlap
` (2 preceding siblings ...)
2015-12-01 11:53 ` [PATCH v3 4/6] xl: Return an error on failed cd-insert George Dunlap
@ 2015-12-01 11:53 ` George Dunlap
2015-12-08 17:32 ` Ian Campbell
2015-12-01 11:53 ` [PATCH v3 6/6] xl: Return error code on save George Dunlap
` (2 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: George Dunlap @ 2015-12-01 11:53 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Ian Jackson, Wei Liu, Ian Campbell
Add return codes for pci-detach, pci-attach, pci-asssignable-add, and
pci-assignable-remove.
Returning error codes makes it easier for shell scripts to tell if a
command has failed or succeeded.
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/xl_cmdimpl.c | 56 ++++++++++++++++++++++++++++++++----------------
1 file changed, 37 insertions(+), 19 deletions(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 72ece2e..5f21c37 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3495,10 +3495,11 @@ int main_pcilist(int argc, char **argv)
return 0;
}
-static void pcidetach(uint32_t domid, const char *bdf, int force)
+static int pcidetach(uint32_t domid, const char *bdf, int force)
{
libxl_device_pci pcidev;
XLU_Config *config;
+ int rc = 0;
libxl_device_pci_init(&pcidev);
@@ -3509,13 +3510,18 @@ static void pcidetach(uint32_t domid, const char *bdf, int force)
fprintf(stderr, "pci-detach: malformed BDF specification \"%s\"\n", bdf);
exit(2);
}
- if (force)
- libxl_device_pci_destroy(ctx, domid, &pcidev, 0);
- else
- libxl_device_pci_remove(ctx, domid, &pcidev, 0);
+ if (force) {
+ if(libxl_device_pci_destroy(ctx, domid, &pcidev, 0))
+ rc = 1;
+ } else {
+ if(libxl_device_pci_remove(ctx, domid, &pcidev, 0))
+ rc = 1;
+ }
libxl_device_pci_dispose(&pcidev);
xlu_cfg_destroy(config);
+
+ return rc;
}
int main_pcidetach(int argc, char **argv)
@@ -3534,13 +3540,14 @@ int main_pcidetach(int argc, char **argv)
domid = find_domain(argv[optind]);
bdf = argv[optind + 1];
- pcidetach(domid, bdf, force);
- return 0;
+ return pcidetach(domid, bdf, force);
}
-static void pciattach(uint32_t domid, const char *bdf, const char *vs)
+
+static int pciattach(uint32_t domid, const char *bdf, const char *vs)
{
libxl_device_pci pcidev;
XLU_Config *config;
+ int rc;
libxl_device_pci_init(&pcidev);
@@ -3551,10 +3558,14 @@ static void pciattach(uint32_t domid, const char *bdf, const char *vs)
fprintf(stderr, "pci-attach: malformed BDF specification \"%s\"\n", bdf);
exit(2);
}
- libxl_device_pci_add(ctx, domid, &pcidev, 0);
+
+ if(libxl_device_pci_add(ctx, domid, &pcidev, 0))
+ rc = 1;
libxl_device_pci_dispose(&pcidev);
xlu_cfg_destroy(config);
+
+ return rc;
}
int main_pciattach(int argc, char **argv)
@@ -3573,8 +3584,7 @@ int main_pciattach(int argc, char **argv)
if (optind + 1 < argc)
vs = argv[optind + 2];
- pciattach(domid, bdf, vs);
- return 0;
+ return pciattach(domid, bdf, vs);
}
static void pciassignable_list(void)
@@ -3606,10 +3616,11 @@ int main_pciassignable_list(int argc, char **argv)
return 0;
}
-static void pciassignable_add(const char *bdf, int rebind)
+static int pciassignable_add(const char *bdf, int rebind)
{
libxl_device_pci pcidev;
XLU_Config *config;
+ int rc = 0;
libxl_device_pci_init(&pcidev);
@@ -3620,10 +3631,14 @@ static void pciassignable_add(const char *bdf, int rebind)
fprintf(stderr, "pci-assignable-add: malformed BDF specification \"%s\"\n", bdf);
exit(2);
}
- libxl_device_pci_assignable_add(ctx, &pcidev, rebind);
+
+ if (libxl_device_pci_assignable_add(ctx, &pcidev, rebind))
+ rc = 1;
libxl_device_pci_dispose(&pcidev);
xlu_cfg_destroy(config);
+
+ return rc;
}
int main_pciassignable_add(int argc, char **argv)
@@ -3637,14 +3652,14 @@ int main_pciassignable_add(int argc, char **argv)
bdf = argv[optind];
- pciassignable_add(bdf, 1);
- return 0;
+ return pciassignable_add(bdf, 1);
}
-static void pciassignable_remove(const char *bdf, int rebind)
+static int pciassignable_remove(const char *bdf, int rebind)
{
libxl_device_pci pcidev;
XLU_Config *config;
+ int rc = 0;
libxl_device_pci_init(&pcidev);
@@ -3655,10 +3670,14 @@ static void pciassignable_remove(const char *bdf, int rebind)
fprintf(stderr, "pci-assignable-remove: malformed BDF specification \"%s\"\n", bdf);
exit(2);
}
- libxl_device_pci_assignable_remove(ctx, &pcidev, rebind);
+
+ if(libxl_device_pci_assignable_remove(ctx, &pcidev, rebind))
+ rc = 1;
libxl_device_pci_dispose(&pcidev);
xlu_cfg_destroy(config);
+
+ return rc;
}
int main_pciassignable_remove(int argc, char **argv)
@@ -3675,8 +3694,7 @@ int main_pciassignable_remove(int argc, char **argv)
bdf = argv[optind];
- pciassignable_remove(bdf, rebind);
- return 0;
+ return pciassignable_remove(bdf, rebind);
}
static void pause_domain(uint32_t domid)
--
2.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3 5/6] xl: Return error codes for pci* commands
2015-12-01 11:53 ` [PATCH v3 5/6] xl: Return error codes for pci* commands George Dunlap
@ 2015-12-08 17:32 ` Ian Campbell
0 siblings, 0 replies; 18+ messages in thread
From: Ian Campbell @ 2015-12-08 17:32 UTC (permalink / raw)
To: George Dunlap, xen-devel; +Cc: Ian Jackson, Wei Liu
On Tue, 2015-12-01 at 11:53 +0000, George Dunlap wrote:
> Add return codes for pci-detach, pci-attach, pci-asssignable-add, and
> pci-assignable-remove.
>
> Returning error codes makes it easier for shell scripts to tell if a
> command has failed or succeeded.
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
> tools/libxl/xl_cmdimpl.c | 56 ++++++++++++++++++++++++++++++++--------
> --------
> 1 file changed, 37 insertions(+), 19 deletions(-)
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 72ece2e..5f21c37 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3495,10 +3495,11 @@ int main_pcilist(int argc, char **argv)
> return 0;
> }
>
> -static void pcidetach(uint32_t domid, const char *bdf, int force)
> +static int pcidetach(uint32_t domid, const char *bdf, int force)
> {
> libxl_device_pci pcidev;
> XLU_Config *config;
> + int rc = 0;
I think we should probably avoid "rc" for non-libxl error codes even in xl.
Also, I'd forgotten that since 00e110e44a0eb we are trying to have xl
main_* functions return either EXIT_SUCCESS or EXIT_FAILURE rather than 0,
1, -foo etc. That probably applies to many of the earlier patches which
I've already commented on. I shan't go back and correct myself.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 6/6] xl: Return error code on save
2015-12-01 11:53 [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach George Dunlap
` (3 preceding siblings ...)
2015-12-01 11:53 ` [PATCH v3 5/6] xl: Return error codes for pci* commands George Dunlap
@ 2015-12-01 11:53 ` George Dunlap
2015-12-08 17:38 ` Ian Campbell
2015-12-16 18:25 ` George Dunlap
2015-12-08 12:24 ` [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach George Dunlap
2015-12-08 17:15 ` Ian Campbell
6 siblings, 2 replies; 18+ messages in thread
From: George Dunlap @ 2015-12-01 11:53 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Ian Jackson, Wei Liu, Ian Campbell
save_domain was already constructing an error code; it just wasn't
being used.
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/xl_cmdimpl.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5f21c37..52aedcf 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4712,8 +4712,7 @@ int main_save(int argc, char **argv)
if ( argc - optind >= 3 )
config_filename = argv[optind + 2];
- save_domain(domid, filename, checkpoint, leavepaused, config_filename);
- return 0;
+ return save_domain(domid, filename, checkpoint, leavepaused, config_filename);
}
int main_migrate(int argc, char **argv)
--
2.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3 6/6] xl: Return error code on save
2015-12-01 11:53 ` [PATCH v3 6/6] xl: Return error code on save George Dunlap
@ 2015-12-08 17:38 ` Ian Campbell
2015-12-16 18:25 ` George Dunlap
1 sibling, 0 replies; 18+ messages in thread
From: Ian Campbell @ 2015-12-08 17:38 UTC (permalink / raw)
To: George Dunlap, xen-devel; +Cc: Ian Jackson, Wei Liu
On Tue, 2015-12-01 at 11:53 +0000, George Dunlap wrote:
> save_domain was already constructing an error code; it just wasn't
> being used.
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
> tools/libxl/xl_cmdimpl.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5f21c37..52aedcf 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4712,8 +4712,7 @@ int main_save(int argc, char **argv)
> if ( argc - optind >= 3 )
> config_filename = argv[optind + 2];
>
> - save_domain(domid, filename, checkpoint, leavepaused,
> config_filename);
> - return 0;
> + return save_domain(domid, filename, checkpoint, leavepaused,
> config_filename);
> }
>
> int main_migrate(int argc, char **argv)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 6/6] xl: Return error code on save
2015-12-01 11:53 ` [PATCH v3 6/6] xl: Return error code on save George Dunlap
2015-12-08 17:38 ` Ian Campbell
@ 2015-12-16 18:25 ` George Dunlap
1 sibling, 0 replies; 18+ messages in thread
From: George Dunlap @ 2015-12-16 18:25 UTC (permalink / raw)
To: George Dunlap, xen-devel; +Cc: Ian Jackson, Wei Liu, Ian Campbell
On 01/12/15 11:53, George Dunlap wrote:
> save_domain was already constructing an error code; it just wasn't
> being used.
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
> tools/libxl/xl_cmdimpl.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5f21c37..52aedcf 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4712,8 +4712,7 @@ int main_save(int argc, char **argv)
> if ( argc - optind >= 3 )
> config_filename = argv[optind + 2];
>
> - save_domain(domid, filename, checkpoint, leavepaused, config_filename);
> - return 0;
> + return save_domain(domid, filename, checkpoint, leavepaused, config_filename);
Ah -- turns out the reason the return value "wasn't being used" was that
save_domain() actually calls exit() itself directly.
I'll drop this patch, as the headline feature (xl failing on the save
failing) appears to be working properly. Maybe we can put this on a
list of clean-up items.
-George
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach
2015-12-01 11:53 [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach George Dunlap
` (4 preceding siblings ...)
2015-12-01 11:53 ` [PATCH v3 6/6] xl: Return error code on save George Dunlap
@ 2015-12-08 12:24 ` George Dunlap
2015-12-08 17:15 ` Ian Campbell
6 siblings, 0 replies; 18+ messages in thread
From: George Dunlap @ 2015-12-08 12:24 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Ian Jackson, Wei Liu, Ian Campbell
On Tue, Dec 1, 2015 at 11:53 AM, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> Return proper error codes on failure so that scripts can tell whether
> the command completed properly or not.
>
> Also while we're at it, normalize init and dispose of
> libxl_device_disk structures. This means calling init and dispose in
> the actual functions where they are declared.
>
> This in turn means changing parse_disk_config_multistring() to not
> call libxl_device_disk_init. And that in turn means changes to
> callers of parse_disk_config().
>
> It also means removing libxl_device_disk_init() from
> libxl.c:libxl_vdev_to_device_disk(). This is only called from
> xl_cmdimpl.c.
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Ping?
-George
> ---
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Konrad Wilk <konrad.wilk@oracle.com>
>
> v3:
> - Rebased to tip
>
> v2:
> - Restructure functions to make sure init and dispose are properly
> called.
> ---
> tools/libxl/libxl.c | 2 --
> tools/libxl/xl_cmdimpl.c | 29 ++++++++++++++++++++---------
> 2 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index bd3aac8..814d056 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2738,8 +2738,6 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t domid,
> if (devid < 0)
> return ERROR_INVAL;
>
> - libxl_device_disk_init(disk);
> -
> dompath = libxl__xs_get_dompath(gc, domid);
> if (!dompath) {
> goto out;
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 2b6371d..2ba2393 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -570,8 +570,6 @@ static void parse_disk_config_multistring(XLU_Config **config,
> {
> int e;
>
> - libxl_device_disk_init(disk);
> -
> if (!*config) {
> *config = xlu_cfg_init(stderr, "command line");
> if (!*config) { perror("xlu_cfg_init"); exit(-1); }
> @@ -3340,6 +3338,8 @@ static int cd_insert(uint32_t domid, const char *virtdev, char *phys)
> xasprintf(&buf, "vdev=%s,access=r,devtype=cdrom,target=%s",
> virtdev, phys ? phys : "");
>
> + libxl_device_disk_init(&disk);
> +
> parse_disk_config(&config, buf, &disk);
>
> /* ATM the existence of the backing file is not checked for qdisk
> @@ -6649,6 +6649,7 @@ int main_blockattach(int argc, char **argv)
> uint32_t fe_domid;
> libxl_device_disk disk;
> XLU_Config *config = 0;
> + int rc = 0;
>
> SWITCH_FOREACH_OPT(opt, "", NULL, "block-attach", 2) {
> /* No options */
> @@ -6660,6 +6661,8 @@ int main_blockattach(int argc, char **argv)
> }
> optind++;
>
> + libxl_device_disk_init(&disk);
> +
> parse_disk_config_multistring
> (&config, argc-optind, (const char* const*)argv + optind, &disk);
>
> @@ -6668,14 +6671,17 @@ int main_blockattach(int argc, char **argv)
> printf("disk: %s\n", json);
> free(json);
> if (ferror(stdout) || fflush(stdout)) { perror("stdout"); exit(-1); }
> - return 0;
> + goto out;
> }
>
> if (libxl_device_disk_add(ctx, fe_domid, &disk, 0)) {
> fprintf(stderr, "libxl_device_disk_add failed.\n");
> - return 1;
> + rc = 1;
> }
> - return 0;
> +out:
> + libxl_device_disk_dispose(&disk);
> +
> + return rc;
> }
>
> int main_blocklist(int argc, char **argv)
> @@ -6726,18 +6732,23 @@ int main_blockdetach(int argc, char **argv)
> /* No options */
> }
>
> + libxl_device_disk_init(&disk);
> +
> domid = find_domain(argv[optind]);
>
> if (libxl_vdev_to_device_disk(ctx, domid, argv[optind+1], &disk)) {
> fprintf(stderr, "Error: Device %s not connected.\n", argv[optind+1]);
> - return 1;
> + rc = 1;
> + goto out;
> }
> - rc = libxl_device_disk_remove(ctx, domid, &disk, 0);
> - if (rc) {
> + if (libxl_device_disk_remove(ctx, domid, &disk, 0)) {
> fprintf(stderr, "libxl_device_disk_remove failed.\n");
> - return 1;
> + rc = 1;
> }
> +
> +out:
> libxl_device_disk_dispose(&disk);
> +
> return rc;
> }
>
> --
> 2.1.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach
2015-12-01 11:53 [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach George Dunlap
` (5 preceding siblings ...)
2015-12-08 12:24 ` [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach George Dunlap
@ 2015-12-08 17:15 ` Ian Campbell
2015-12-16 16:53 ` George Dunlap
2016-01-04 14:30 ` Wei Liu
6 siblings, 2 replies; 18+ messages in thread
From: Ian Campbell @ 2015-12-08 17:15 UTC (permalink / raw)
To: George Dunlap, xen-devel; +Cc: Ian Jackson, Wei Liu
On Tue, 2015-12-01 at 11:53 +0000, George Dunlap wrote:
> Return proper error codes on failure so that scripts can tell whether
> the command completed properly or not.
>
> Also while we're at it, normalize init and dispose of
> libxl_device_disk structures. This means calling init and dispose in
> the actual functions where they are declared.
>
> This in turn means changing parse_disk_config_multistring() to not
> call libxl_device_disk_init. And that in turn means changes to
> callers of parse_disk_config().
>
> It also means removing libxl_device_disk_init() from
> libxl.c:libxl_vdev_to_device_disk(). This is only called from
> xl_cmdimpl.c.
... and the ocaml bindings.
I can't remember what we decided regarding libxl "getter" functions and
initialisation of the data type (i.e. whose responsibility it is), but it
seems that changing a given calls semantics is rather dangerous API-wise.
Wei, ISTR you stumbling over this once and the formulation of A Plan(tm),
but I can't remember what it was, can you?
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Konrad Wilk <konrad.wilk@oracle.com>
>
> v3:
> - Rebased to tip
>
> v2:
> - Restructure functions to make sure init and dispose are properly
> called.
> ---
> tools/libxl/libxl.c | 2 --
> tools/libxl/xl_cmdimpl.c | 29 ++++++++++++++++++++---------
> 2 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index bd3aac8..814d056 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2738,8 +2738,6 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx,
> uint32_t domid,
> if (devid < 0)
> return ERROR_INVAL;
>
> - libxl_device_disk_init(disk);
> -
> dompath = libxl__xs_get_dompath(gc, domid);
> if (!dompath) {
> goto out;
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 2b6371d..2ba2393 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -570,8 +570,6 @@ static void parse_disk_config_multistring(XLU_Config
> **config,
> {
> int e;
>
> - libxl_device_disk_init(disk);
> -
> if (!*config) {
> *config = xlu_cfg_init(stderr, "command line");
> if (!*config) { perror("xlu_cfg_init"); exit(-1); }
> @@ -3340,6 +3338,8 @@ static int cd_insert(uint32_t domid, const char
> *virtdev, char *phys)
> xasprintf(&buf, "vdev=%s,access=r,devtype=cdrom,target=%s",
> virtdev, phys ? phys : "");
>
> + libxl_device_disk_init(&disk);
> +
> parse_disk_config(&config, buf, &disk);
>
> /* ATM the existence of the backing file is not checked for qdisk
> @@ -6649,6 +6649,7 @@ int main_blockattach(int argc, char **argv)
> uint32_t fe_domid;
> libxl_device_disk disk;
> XLU_Config *config = 0;
> + int rc = 0;
>
> SWITCH_FOREACH_OPT(opt, "", NULL, "block-attach", 2) {
> /* No options */
> @@ -6660,6 +6661,8 @@ int main_blockattach(int argc, char **argv)
> }
> optind++;
>
> + libxl_device_disk_init(&disk);
> +
> parse_disk_config_multistring
> (&config, argc-optind, (const char* const*)argv + optind,
> &disk);
>
> @@ -6668,14 +6671,17 @@ int main_blockattach(int argc, char **argv)
> printf("disk: %s\n", json);
> free(json);
> if (ferror(stdout) || fflush(stdout)) { perror("stdout"); exit(-
> 1); }
> - return 0;
> + goto out;
> }
>
> if (libxl_device_disk_add(ctx, fe_domid, &disk, 0)) {
> fprintf(stderr, "libxl_device_disk_add failed.\n");
> - return 1;
> + rc = 1;
> }
> - return 0;
> +out:
> + libxl_device_disk_dispose(&disk);
> +
> + return rc;
> }
>
> int main_blocklist(int argc, char **argv)
> @@ -6726,18 +6732,23 @@ int main_blockdetach(int argc, char **argv)
> /* No options */
> }
>
> + libxl_device_disk_init(&disk);
> +
> domid = find_domain(argv[optind]);
>
> if (libxl_vdev_to_device_disk(ctx, domid, argv[optind+1], &disk)) {
> fprintf(stderr, "Error: Device %s not connected.\n",
> argv[optind+1]);
> - return 1;
> + rc = 1;
> + goto out;
> }
> - rc = libxl_device_disk_remove(ctx, domid, &disk, 0);
> - if (rc) {
> + if (libxl_device_disk_remove(ctx, domid, &disk, 0)) {
> fprintf(stderr, "libxl_device_disk_remove failed.\n");
> - return 1;
> + rc = 1;
> }
> +
> +out:
> libxl_device_disk_dispose(&disk);
> +
> return rc;
> }
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach
2015-12-08 17:15 ` Ian Campbell
@ 2015-12-16 16:53 ` George Dunlap
2015-12-16 17:13 ` George Dunlap
2016-01-04 14:30 ` Wei Liu
1 sibling, 1 reply; 18+ messages in thread
From: George Dunlap @ 2015-12-16 16:53 UTC (permalink / raw)
To: Ian Campbell, George Dunlap, xen-devel; +Cc: Ian Jackson, Wei Liu
On 08/12/15 17:15, Ian Campbell wrote:
> On Tue, 2015-12-01 at 11:53 +0000, George Dunlap wrote:
>> Return proper error codes on failure so that scripts can tell whether
>> the command completed properly or not.
>>
>> Also while we're at it, normalize init and dispose of
>> libxl_device_disk structures. This means calling init and dispose in
>> the actual functions where they are declared.
>>
>> This in turn means changing parse_disk_config_multistring() to not
>> call libxl_device_disk_init. And that in turn means changes to
>> callers of parse_disk_config().
>>
>> It also means removing libxl_device_disk_init() from
>> libxl.c:libxl_vdev_to_device_disk(). This is only called from
>> xl_cmdimpl.c.
>
> ... and the ocaml bindings.
>
> I can't remember what we decided regarding libxl "getter" functions and
> initialisation of the data type (i.e. whose responsibility it is), but it
> seems that changing a given calls semantics is rather dangerous API-wise.
Right -- looks like there are similar issues with the nic lookup
routines as well. I'll drop that bit from the patch.
-George
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach
2015-12-16 16:53 ` George Dunlap
@ 2015-12-16 17:13 ` George Dunlap
0 siblings, 0 replies; 18+ messages in thread
From: George Dunlap @ 2015-12-16 17:13 UTC (permalink / raw)
To: Ian Campbell, George Dunlap, xen-devel; +Cc: Ian Jackson, Wei Liu
On 16/12/15 16:53, George Dunlap wrote:
> On 08/12/15 17:15, Ian Campbell wrote:
>> On Tue, 2015-12-01 at 11:53 +0000, George Dunlap wrote:
>>> Return proper error codes on failure so that scripts can tell whether
>>> the command completed properly or not.
>>>
>>> Also while we're at it, normalize init and dispose of
>>> libxl_device_disk structures. This means calling init and dispose in
>>> the actual functions where they are declared.
>>>
>>> This in turn means changing parse_disk_config_multistring() to not
>>> call libxl_device_disk_init. And that in turn means changes to
>>> callers of parse_disk_config().
>>>
>>> It also means removing libxl_device_disk_init() from
>>> libxl.c:libxl_vdev_to_device_disk(). This is only called from
>>> xl_cmdimpl.c.
>>
>> ... and the ocaml bindings.
>>
>> I can't remember what we decided regarding libxl "getter" functions and
>> initialisation of the data type (i.e. whose responsibility it is), but it
>> seems that changing a given calls semantics is rather dangerous API-wise.
>
> Right -- looks like there are similar issues with the nic lookup
> routines as well. I'll drop that bit from the patch.
Hmm -- and upon further inspection, it appears that the headline feature
(returning appropriate error codes) was already checked in. What was
left was only the "while we're at it" bit. :-/
I'll drop this from the series...
-George
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/6] xl: Return proper error codes for block-attach and block-detach
2015-12-08 17:15 ` Ian Campbell
2015-12-16 16:53 ` George Dunlap
@ 2016-01-04 14:30 ` Wei Liu
1 sibling, 0 replies; 18+ messages in thread
From: Wei Liu @ 2016-01-04 14:30 UTC (permalink / raw)
To: Ian Campbell; +Cc: George Dunlap, Ian Jackson, Wei Liu, xen-devel
On Tue, Dec 08, 2015 at 05:15:28PM +0000, Ian Campbell wrote:
> On Tue, 2015-12-01 at 11:53 +0000, George Dunlap wrote:
> > Return proper error codes on failure so that scripts can tell whether
> > the command completed properly or not.
> >
> > Also while we're at it, normalize init and dispose of
> > libxl_device_disk structures. This means calling init and dispose in
> > the actual functions where they are declared.
> >
> > This in turn means changing parse_disk_config_multistring() to not
> > call libxl_device_disk_init. And that in turn means changes to
> > callers of parse_disk_config().
> >
> > It also means removing libxl_device_disk_init() from
> > libxl.c:libxl_vdev_to_device_disk(). This is only called from
> > xl_cmdimpl.c.
>
> ... and the ocaml bindings.
>
> I can't remember what we decided regarding libxl "getter" functions and
> initialisation of the data type (i.e. whose responsibility it is), but it
> seems that changing a given calls semantics is rather dangerous API-wise.
>
> Wei, ISTR you stumbling over this once and the formulation of A Plan(tm),
> but I can't remember what it was, can you?
>
I vaguely remember getting into something about libxl_device_disk_init
when I was discussing with with Jim regarding libvirt, but I can't
remember exactly what. It's probably a different issue.
Wei.
^ permalink raw reply [flat|nested] 18+ messages in thread