* [PATCH 0/9] xl/libxl: fix issues discovered by Coverity scan
@ 2015-07-10 18:00 Wei Liu
2015-07-10 18:00 ` [PATCH 1/9] libxl: fix libxl__abs_path Wei Liu
` (9 more replies)
0 siblings, 10 replies; 34+ messages in thread
From: Wei Liu @ 2015-07-10 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell, Wei Liu
Wei Liu (9):
libxl: fix libxl__abs_path
libxl: turn two malloc's to libxl__malloc
libxl: json string object can be NULL
libxl: dispose dominfo to avoid leaking resource
libxl: avoid leaking string in cpupool_info
libxl: localtime(3) can return NULL
libxl: qmp_init_handler can return NULL
xl: fix main_cpupoolcreate
xl: fix main_config_update
tools/libxl/libxl.c | 4 +++-
tools/libxl/libxl_aoutils.c | 3 +--
tools/libxl/libxl_device.c | 2 ++
tools/libxl/libxl_dm.c | 2 +-
tools/libxl/libxl_internal.c | 4 ++--
tools/libxl/libxl_json.c | 9 +++++++--
tools/libxl/libxl_qmp.c | 1 +
tools/libxl/libxl_x86.c | 6 ++++++
tools/libxl/xl_cmdimpl.c | 5 +++--
9 files changed, 26 insertions(+), 10 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/9] libxl: fix libxl__abs_path
2015-07-10 18:00 [PATCH 0/9] xl/libxl: fix issues discovered by Coverity scan Wei Liu
@ 2015-07-10 18:00 ` Wei Liu
2015-07-13 9:57 ` Ian Campbell
2015-07-10 18:00 ` [PATCH 2/9] libxl: turn two malloc's to libxl__malloc Wei Liu
` (8 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Wei Liu @ 2015-07-10 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell, Wei Liu
If s is NULL, just return NULL to avoid libxl__strdup dereferencing NULL
pointer.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/libxl_internal.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 42d548e..6402c1b 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -233,8 +233,8 @@ void libxl__log(libxl_ctx *ctx, xentoollog_level msglevel, int errnoval,
char *libxl__abs_path(libxl__gc *gc, const char *s, const char *path)
{
- if (!s || s[0] == '/')
- return libxl__strdup(gc, s);
+ if (!s) return NULL;
+ if (s[0] == '/') return libxl__strdup(gc, s);
return libxl__sprintf(gc, "%s/%s", path, s);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 2/9] libxl: turn two malloc's to libxl__malloc
2015-07-10 18:00 [PATCH 0/9] xl/libxl: fix issues discovered by Coverity scan Wei Liu
2015-07-10 18:00 ` [PATCH 1/9] libxl: fix libxl__abs_path Wei Liu
@ 2015-07-10 18:00 ` Wei Liu
2015-07-13 10:00 ` Ian Campbell
2015-07-10 18:00 ` [PATCH 3/9] libxl: json string object can be NULL Wei Liu
` (7 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Wei Liu @ 2015-07-10 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell, Wei Liu
One is to combine malloc + libxl__alloc_failed. The other is to avoid
dereferencing NULL pointer in case of malloc failure.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/libxl_aoutils.c | 3 +--
tools/libxl/libxl_dm.c | 2 +-
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 0931eee..0300396 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -245,8 +245,7 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
buf = LIBXL_TAILQ_LAST(&dc->bufs, libxl__datacopier_bufs);
if (!buf || buf->used >= sizeof(buf->buf)) {
- buf = malloc(sizeof(*buf));
- if (!buf) libxl__alloc_failed(CTX, __func__, 1, sizeof(*buf));
+ buf = libxl__malloc(NOGC, sizeof(*buf));
buf->used = 0;
LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
}
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index ad434f0..0cc73be 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1010,7 +1010,7 @@ static int libxl__write_stub_dmargs(libxl__gc *gc,
i++;
}
dmargs_size++;
- dmargs = (char *) malloc(dmargs_size);
+ dmargs = (char *) libxl__malloc(NOGC, dmargs_size);
i = 1;
dmargs[0] = '\0';
while (args[i] != NULL) {
--
1.9.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 3/9] libxl: json string object can be NULL
2015-07-10 18:00 [PATCH 0/9] xl/libxl: fix issues discovered by Coverity scan Wei Liu
2015-07-10 18:00 ` [PATCH 1/9] libxl: fix libxl__abs_path Wei Liu
2015-07-10 18:00 ` [PATCH 2/9] libxl: turn two malloc's to libxl__malloc Wei Liu
@ 2015-07-10 18:00 ` Wei Liu
2015-07-13 10:02 ` Ian Campbell
2015-07-10 18:00 ` [PATCH 4/9] libxl: dispose dominfo to avoid leaking resource Wei Liu
` (6 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Wei Liu @ 2015-07-10 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell, Wei Liu
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/libxl_json.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index 346929a..652b3f4 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -433,8 +433,13 @@ int libxl__string_parse_json(libxl__gc *gc, const libxl__json_object *o,
if (libxl__json_object_is_null(o))
*p = NULL;
- else
- *p = libxl__strdup(NOGC, libxl__json_object_get_string(o));
+ else {
+ const char *s = libxl__json_object_get_string(o);
+ if (!s)
+ *p = NULL;
+ else
+ *p = libxl__strdup(NOGC, s);
+ }
return 0;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 4/9] libxl: dispose dominfo to avoid leaking resource
2015-07-10 18:00 [PATCH 0/9] xl/libxl: fix issues discovered by Coverity scan Wei Liu
` (2 preceding siblings ...)
2015-07-10 18:00 ` [PATCH 3/9] libxl: json string object can be NULL Wei Liu
@ 2015-07-10 18:00 ` Wei Liu
2015-07-13 10:05 ` Ian Campbell
2015-07-10 18:00 ` [PATCH 5/9] libxl: avoid leaking string in cpupool_info Wei Liu
` (5 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Wei Liu @ 2015-07-10 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell, Wei Liu
Add libxl_dominfo_dispose to one return path that doesn't have it.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/libxl_device.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 2493972..3f8b555 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -816,6 +816,8 @@ void libxl__initiate_device_remove(libxl__egc *egc,
be_path);
goto out;
}
+
+ libxl_dominfo_dispose(&info);
return;
}
}
--
1.9.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 5/9] libxl: avoid leaking string in cpupool_info
2015-07-10 18:00 [PATCH 0/9] xl/libxl: fix issues discovered by Coverity scan Wei Liu
` (3 preceding siblings ...)
2015-07-10 18:00 ` [PATCH 4/9] libxl: dispose dominfo to avoid leaking resource Wei Liu
@ 2015-07-10 18:00 ` Wei Liu
2015-07-13 10:07 ` Ian Campbell
2015-07-10 18:00 ` [PATCH 6/9] libxl: localtime(3) can return NULL Wei Liu
` (4 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Wei Liu @ 2015-07-10 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell, Wei Liu
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/libxl.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 38aff8d..4151dcb 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -740,8 +740,10 @@ static int cpupool_info(libxl__gc *gc,
info->sched = xcinfo->sched_id;
info->n_dom = xcinfo->n_dom;
rc = libxl_cpu_bitmap_alloc(CTX, &info->cpumap, 0);
- if (rc)
+ if (rc) {
+ free(info->pool_name);
goto out;
+ }
memcpy(info->cpumap.map, xcinfo->cpumap, info->cpumap.size);
--
1.9.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 6/9] libxl: localtime(3) can return NULL
2015-07-10 18:00 [PATCH 0/9] xl/libxl: fix issues discovered by Coverity scan Wei Liu
` (4 preceding siblings ...)
2015-07-10 18:00 ` [PATCH 5/9] libxl: avoid leaking string in cpupool_info Wei Liu
@ 2015-07-10 18:00 ` Wei Liu
2015-07-13 10:09 ` Ian Campbell
2015-07-10 18:00 ` [PATCH 7/9] libxl: qmp_init_handler " Wei Liu
` (3 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Wei Liu @ 2015-07-10 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell, Wei Liu
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/libxl_x86.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 8cd15ca..6be9b1b 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -311,6 +311,11 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
t = time(NULL);
tm = localtime(&t);
+ if (!tm) {
+ ret = ERROR_FAIL;
+ goto out;
+ }
+
rtc_timeoffset += tm->tm_gmtoff;
}
@@ -335,6 +340,7 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
}
}
+out:
return ret;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 7/9] libxl: qmp_init_handler can return NULL
2015-07-10 18:00 [PATCH 0/9] xl/libxl: fix issues discovered by Coverity scan Wei Liu
` (5 preceding siblings ...)
2015-07-10 18:00 ` [PATCH 6/9] libxl: localtime(3) can return NULL Wei Liu
@ 2015-07-10 18:00 ` Wei Liu
2015-07-13 10:10 ` Ian Campbell
2015-07-10 18:00 ` [PATCH 8/9] xl: fix main_cpupoolcreate Wei Liu
` (2 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Wei Liu @ 2015-07-10 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell, Wei Liu
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/libxl_qmp.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 6484f5e..965c507 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -694,6 +694,7 @@ libxl__qmp_handler *libxl__qmp_initialize(libxl__gc *gc, uint32_t domid)
char *qmp_socket;
qmp = qmp_init_handler(gc, domid);
+ if (!qmp) return NULL;
qmp_socket = GCSPRINTF("%s/qmp-libxl-%d", libxl__run_dir_path(), domid);
if ((ret = qmp_open(qmp, qmp_socket, QMP_SOCKET_CONNECT_TIMEOUT)) < 0) {
--
1.9.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 8/9] xl: fix main_cpupoolcreate
2015-07-10 18:00 [PATCH 0/9] xl/libxl: fix issues discovered by Coverity scan Wei Liu
` (6 preceding siblings ...)
2015-07-10 18:00 ` [PATCH 7/9] libxl: qmp_init_handler " Wei Liu
@ 2015-07-10 18:00 ` Wei Liu
2015-07-13 10:11 ` Ian Campbell
2015-07-10 18:00 ` [PATCH 9/9] xl: fix main_config_update Wei Liu
2015-07-10 18:21 ` [PATCH 0/9] xl/libxl: fix issues discovered by Coverity scan Andrew Cooper
9 siblings, 1 reply; 34+ messages in thread
From: Wei Liu @ 2015-07-10 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell, Wei Liu
Don't dereference extra_config if it's NULL. Don't leak extra_config in
the end.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/xl_cmdimpl.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 971209c..d44eb4b 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7228,7 +7228,7 @@ int main_cpupoolcreate(int argc, char **argv)
else
config_src="command line";
- if (strlen(extra_config)) {
+ if (extra_config && strlen(extra_config)) {
if (config_len > INT_MAX - (strlen(extra_config) + 2)) {
fprintf(stderr, "Failed to attach extra configration\n");
goto out;
@@ -7365,6 +7365,7 @@ out_cfg:
out:
free(name);
free(config_data);
+ free(extra_config);
return rc;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 9/9] xl: fix main_config_update
2015-07-10 18:00 [PATCH 0/9] xl/libxl: fix issues discovered by Coverity scan Wei Liu
` (7 preceding siblings ...)
2015-07-10 18:00 ` [PATCH 8/9] xl: fix main_cpupoolcreate Wei Liu
@ 2015-07-10 18:00 ` Wei Liu
2015-07-13 10:12 ` Ian Campbell
2015-07-10 18:21 ` [PATCH 0/9] xl/libxl: fix issues discovered by Coverity scan Andrew Cooper
9 siblings, 1 reply; 34+ messages in thread
From: Wei Liu @ 2015-07-10 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell, Wei Liu
Don't dereference NULL.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/xl_cmdimpl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index d44eb4b..631dbd1 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5010,7 +5010,7 @@ int main_config_update(int argc, char **argv)
if (rc) { fprintf(stderr, "Failed to read config file: %s: %s\n",
filename, strerror(errno));
free(extra_config); return ERROR_FAIL; }
- if (strlen(extra_config)) {
+ if (extra_config && strlen(extra_config)) {
if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
fprintf(stderr, "Failed to attach extra configration\n");
exit(1);
--
1.9.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 0/9] xl/libxl: fix issues discovered by Coverity scan
2015-07-10 18:00 [PATCH 0/9] xl/libxl: fix issues discovered by Coverity scan Wei Liu
` (8 preceding siblings ...)
2015-07-10 18:00 ` [PATCH 9/9] xl: fix main_config_update Wei Liu
@ 2015-07-10 18:21 ` Andrew Cooper
9 siblings, 0 replies; 34+ messages in thread
From: Andrew Cooper @ 2015-07-10 18:21 UTC (permalink / raw)
To: Wei Liu, Xen-devel; +Cc: Ian Jackson, Ian Campbell
On 10/07/15 19:00, Wei Liu wrote:
> Wei Liu (9):
> libxl: fix libxl__abs_path
> libxl: turn two malloc's to libxl__malloc
> libxl: json string object can be NULL
> libxl: dispose dominfo to avoid leaking resource
> libxl: avoid leaking string in cpupool_info
> libxl: localtime(3) can return NULL
> libxl: qmp_init_handler can return NULL
> xl: fix main_cpupoolcreate
> xl: fix main_config_update
Correlating with Coverity Scan, the only defect in common is
1/9 - Coverity-ID: 1198722
All other defects are only found in XenServers instance (we do have all
the knobs turned up to 11).
~Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/9] libxl: fix libxl__abs_path
2015-07-10 18:00 ` [PATCH 1/9] libxl: fix libxl__abs_path Wei Liu
@ 2015-07-13 9:57 ` Ian Campbell
2015-07-13 10:00 ` Wei Liu
2015-07-13 17:12 ` Ian Jackson
0 siblings, 2 replies; 34+ messages in thread
From: Ian Campbell @ 2015-07-13 9:57 UTC (permalink / raw)
To: Wei Liu; +Cc: Xen-devel, Ian Jackson, Andrew Cooper
On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
I rather dislike subjects of the form "fix $function", since it gives
very little clue to someone reading the shortlog what is going on.
In this case I think "libxl: make libxl__abs_path correctly handle a
NULL argument" would be an accurate description.
> If s is NULL, just return NULL to avoid libxl__strdup dereferencing NULL
> pointer.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
For the change itself:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> tools/libxl/libxl_internal.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index 42d548e..6402c1b 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -233,8 +233,8 @@ void libxl__log(libxl_ctx *ctx, xentoollog_level msglevel, int errnoval,
>
> char *libxl__abs_path(libxl__gc *gc, const char *s, const char *path)
> {
> - if (!s || s[0] == '/')
> - return libxl__strdup(gc, s);
> + if (!s) return NULL;
> + if (s[0] == '/') return libxl__strdup(gc, s);
> return libxl__sprintf(gc, "%s/%s", path, s);
> }
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/9] libxl: turn two malloc's to libxl__malloc
2015-07-10 18:00 ` [PATCH 2/9] libxl: turn two malloc's to libxl__malloc Wei Liu
@ 2015-07-13 10:00 ` Ian Campbell
2015-07-13 15:29 ` Wei Liu
0 siblings, 1 reply; 34+ messages in thread
From: Ian Campbell @ 2015-07-13 10:00 UTC (permalink / raw)
To: Wei Liu; +Cc: Xen-devel, Ian Jackson, Andrew Cooper
On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> One is to combine malloc + libxl__alloc_failed. The other is to avoid
> dereferencing NULL pointer in case of malloc failure.
The non-use of a gc for the latter in particular looks a bit suspicious
to me, but nonetheless this is an improvement:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> tools/libxl/libxl_aoutils.c | 3 +--
> tools/libxl/libxl_dm.c | 2 +-
> 2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> index 0931eee..0300396 100644
> --- a/tools/libxl/libxl_aoutils.c
> +++ b/tools/libxl/libxl_aoutils.c
> @@ -245,8 +245,7 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
>
> buf = LIBXL_TAILQ_LAST(&dc->bufs, libxl__datacopier_bufs);
> if (!buf || buf->used >= sizeof(buf->buf)) {
> - buf = malloc(sizeof(*buf));
> - if (!buf) libxl__alloc_failed(CTX, __func__, 1, sizeof(*buf));
> + buf = libxl__malloc(NOGC, sizeof(*buf));
> buf->used = 0;
> LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
> }
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index ad434f0..0cc73be 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1010,7 +1010,7 @@ static int libxl__write_stub_dmargs(libxl__gc *gc,
> i++;
> }
> dmargs_size++;
> - dmargs = (char *) malloc(dmargs_size);
> + dmargs = (char *) libxl__malloc(NOGC, dmargs_size);
> i = 1;
> dmargs[0] = '\0';
> while (args[i] != NULL) {
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/9] libxl: fix libxl__abs_path
2015-07-13 9:57 ` Ian Campbell
@ 2015-07-13 10:00 ` Wei Liu
2015-07-13 17:12 ` Ian Jackson
1 sibling, 0 replies; 34+ messages in thread
From: Wei Liu @ 2015-07-13 10:00 UTC (permalink / raw)
To: Ian Campbell; +Cc: Xen-devel, Wei Liu, Ian Jackson, Andrew Cooper
On Mon, Jul 13, 2015 at 10:57:32AM +0100, Ian Campbell wrote:
> On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
>
> I rather dislike subjects of the form "fix $function", since it gives
> very little clue to someone reading the shortlog what is going on.
>
> In this case I think "libxl: make libxl__abs_path correctly handle a
> NULL argument" would be an accurate description.
Ack.
I can resend the whole series after updating subject for patch #1, #8
and #9.
Wei.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/9] libxl: json string object can be NULL
2015-07-10 18:00 ` [PATCH 3/9] libxl: json string object can be NULL Wei Liu
@ 2015-07-13 10:02 ` Ian Campbell
2015-07-13 17:16 ` Ian Jackson
0 siblings, 1 reply; 34+ messages in thread
From: Ian Campbell @ 2015-07-13 10:02 UTC (permalink / raw)
To: Wei Liu; +Cc: Xen-devel, Ian Jackson, Andrew Cooper
On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
It occurs to me that since libxl__strdup can never return NULL due to
allocation failure we could consider make libxl__strdup(gc, NULL) be
well defined as returning NULL.
> ---
> tools/libxl/libxl_json.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
> index 346929a..652b3f4 100644
> --- a/tools/libxl/libxl_json.c
> +++ b/tools/libxl/libxl_json.c
> @@ -433,8 +433,13 @@ int libxl__string_parse_json(libxl__gc *gc, const libxl__json_object *o,
>
> if (libxl__json_object_is_null(o))
> *p = NULL;
> - else
> - *p = libxl__strdup(NOGC, libxl__json_object_get_string(o));
> + else {
> + const char *s = libxl__json_object_get_string(o);
> + if (!s)
> + *p = NULL;
> + else
> + *p = libxl__strdup(NOGC, s);
> + }
>
> return 0;
> }
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/9] libxl: dispose dominfo to avoid leaking resource
2015-07-10 18:00 ` [PATCH 4/9] libxl: dispose dominfo to avoid leaking resource Wei Liu
@ 2015-07-13 10:05 ` Ian Campbell
2015-07-13 17:18 ` Ian Jackson
0 siblings, 1 reply; 34+ messages in thread
From: Ian Campbell @ 2015-07-13 10:05 UTC (permalink / raw)
To: Wei Liu; +Cc: Xen-devel, Ian Jackson, Andrew Cooper
On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> Add libxl_dominfo_dispose to one return path that doesn't have it.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
That return is a bit at odds with the generally correct error handling
in that function, but this improves things at least a little and I can
sort of see why this a slightly special case, so:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> tools/libxl/libxl_device.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 2493972..3f8b555 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -816,6 +816,8 @@ void libxl__initiate_device_remove(libxl__egc *egc,
> be_path);
> goto out;
> }
> +
> + libxl_dominfo_dispose(&info);
> return;
> }
> }
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 5/9] libxl: avoid leaking string in cpupool_info
2015-07-10 18:00 ` [PATCH 5/9] libxl: avoid leaking string in cpupool_info Wei Liu
@ 2015-07-13 10:07 ` Ian Campbell
2015-07-13 16:10 ` Wei Liu
0 siblings, 1 reply; 34+ messages in thread
From: Ian Campbell @ 2015-07-13 10:07 UTC (permalink / raw)
To: Wei Liu; +Cc: Xen-devel, Ian Jackson, Andrew Cooper
On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Would an "if (rc) libxl_cpupoolinfo_dispose(info)" on the exit path be a
more robust alternative? Might require the addition of a
libxl_cpupoolinfo_init() somewhere before any possible error.
> ---
> tools/libxl/libxl.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 38aff8d..4151dcb 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -740,8 +740,10 @@ static int cpupool_info(libxl__gc *gc,
> info->sched = xcinfo->sched_id;
> info->n_dom = xcinfo->n_dom;
> rc = libxl_cpu_bitmap_alloc(CTX, &info->cpumap, 0);
> - if (rc)
> + if (rc) {
> + free(info->pool_name);
> goto out;
> + }
>
> memcpy(info->cpumap.map, xcinfo->cpumap, info->cpumap.size);
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 6/9] libxl: localtime(3) can return NULL
2015-07-10 18:00 ` [PATCH 6/9] libxl: localtime(3) can return NULL Wei Liu
@ 2015-07-13 10:09 ` Ian Campbell
0 siblings, 0 replies; 34+ messages in thread
From: Ian Campbell @ 2015-07-13 10:09 UTC (permalink / raw)
To: Wei Liu; +Cc: Xen-devel, Ian Jackson, Andrew Cooper
On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 7/9] libxl: qmp_init_handler can return NULL
2015-07-10 18:00 ` [PATCH 7/9] libxl: qmp_init_handler " Wei Liu
@ 2015-07-13 10:10 ` Ian Campbell
0 siblings, 0 replies; 34+ messages in thread
From: Ian Campbell @ 2015-07-13 10:10 UTC (permalink / raw)
To: Wei Liu; +Cc: Xen-devel, Ian Jackson, Andrew Cooper
On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
(although the only actual reason for a failure today is a memory
allocation failure, which ought to abort really).
> ---
> tools/libxl/libxl_qmp.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index 6484f5e..965c507 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -694,6 +694,7 @@ libxl__qmp_handler *libxl__qmp_initialize(libxl__gc *gc, uint32_t domid)
> char *qmp_socket;
>
> qmp = qmp_init_handler(gc, domid);
> + if (!qmp) return NULL;
>
> qmp_socket = GCSPRINTF("%s/qmp-libxl-%d", libxl__run_dir_path(), domid);
> if ((ret = qmp_open(qmp, qmp_socket, QMP_SOCKET_CONNECT_TIMEOUT)) < 0) {
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 8/9] xl: fix main_cpupoolcreate
2015-07-10 18:00 ` [PATCH 8/9] xl: fix main_cpupoolcreate Wei Liu
@ 2015-07-13 10:11 ` Ian Campbell
0 siblings, 0 replies; 34+ messages in thread
From: Ian Campbell @ 2015-07-13 10:11 UTC (permalink / raw)
To: Wei Liu; +Cc: Xen-devel, Ian Jackson, Andrew Cooper
On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> Don't dereference extra_config if it's NULL. Don't leak extra_config in
> the end.
Subject should be more descriptive. "xl: correct handling of
extra_config in main_cpupoolcreate" perhaps? (It's a lot easier to write
non-vague messages for patches which only do one thing)
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> tools/libxl/xl_cmdimpl.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 971209c..d44eb4b 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -7228,7 +7228,7 @@ int main_cpupoolcreate(int argc, char **argv)
> else
> config_src="command line";
>
> - if (strlen(extra_config)) {
> + if (extra_config && strlen(extra_config)) {
> if (config_len > INT_MAX - (strlen(extra_config) + 2)) {
> fprintf(stderr, "Failed to attach extra configration\n");
There's a typo in this line of context...
> goto out;
> @@ -7365,6 +7365,7 @@ out_cfg:
> out:
> free(name);
> free(config_data);
> + free(extra_config);
> return rc;
> }
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 9/9] xl: fix main_config_update
2015-07-10 18:00 ` [PATCH 9/9] xl: fix main_config_update Wei Liu
@ 2015-07-13 10:12 ` Ian Campbell
0 siblings, 0 replies; 34+ messages in thread
From: Ian Campbell @ 2015-07-13 10:12 UTC (permalink / raw)
To: Wei Liu; +Cc: Xen-devel, Ian Jackson, Andrew Cooper
On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> Don't dereference NULL.
Subject: xl: correctly handle null extra config in main_config_update
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> tools/libxl/xl_cmdimpl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index d44eb4b..631dbd1 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -5010,7 +5010,7 @@ int main_config_update(int argc, char **argv)
> if (rc) { fprintf(stderr, "Failed to read config file: %s: %s\n",
> filename, strerror(errno));
> free(extra_config); return ERROR_FAIL; }
> - if (strlen(extra_config)) {
> + if (extra_config && strlen(extra_config)) {
> if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
> fprintf(stderr, "Failed to attach extra configration\n");
> exit(1);
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/9] libxl: turn two malloc's to libxl__malloc
2015-07-13 10:00 ` Ian Campbell
@ 2015-07-13 15:29 ` Wei Liu
0 siblings, 0 replies; 34+ messages in thread
From: Wei Liu @ 2015-07-13 15:29 UTC (permalink / raw)
To: Ian Campbell; +Cc: Xen-devel, Wei Liu, Ian Jackson, Andrew Cooper
On Mon, Jul 13, 2015 at 11:00:15AM +0100, Ian Campbell wrote:
> On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> > One is to combine malloc + libxl__alloc_failed. The other is to avoid
> > dereferencing NULL pointer in case of malloc failure.
>
> The non-use of a gc for the latter in particular looks a bit suspicious
> to me, but nonetheless this is an improvement:
>
There is a free() later in this function. I wanted to make this patch
minimum so I didn't switch to using gc and delete that free.
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Thanks.
Wei.
>
> > ---
> > tools/libxl/libxl_aoutils.c | 3 +--
> > tools/libxl/libxl_dm.c | 2 +-
> > 2 files changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> > index 0931eee..0300396 100644
> > --- a/tools/libxl/libxl_aoutils.c
> > +++ b/tools/libxl/libxl_aoutils.c
> > @@ -245,8 +245,7 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
> >
> > buf = LIBXL_TAILQ_LAST(&dc->bufs, libxl__datacopier_bufs);
> > if (!buf || buf->used >= sizeof(buf->buf)) {
> > - buf = malloc(sizeof(*buf));
> > - if (!buf) libxl__alloc_failed(CTX, __func__, 1, sizeof(*buf));
> > + buf = libxl__malloc(NOGC, sizeof(*buf));
> > buf->used = 0;
> > LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
> > }
> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index ad434f0..0cc73be 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -1010,7 +1010,7 @@ static int libxl__write_stub_dmargs(libxl__gc *gc,
> > i++;
> > }
> > dmargs_size++;
> > - dmargs = (char *) malloc(dmargs_size);
> > + dmargs = (char *) libxl__malloc(NOGC, dmargs_size);
> > i = 1;
> > dmargs[0] = '\0';
> > while (args[i] != NULL) {
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 5/9] libxl: avoid leaking string in cpupool_info
2015-07-13 10:07 ` Ian Campbell
@ 2015-07-13 16:10 ` Wei Liu
2015-07-13 16:23 ` Ian Campbell
0 siblings, 1 reply; 34+ messages in thread
From: Wei Liu @ 2015-07-13 16:10 UTC (permalink / raw)
To: Ian Campbell; +Cc: Xen-devel, Wei Liu, Ian Jackson, Andrew Cooper
On Mon, Jul 13, 2015 at 11:07:31AM +0100, Ian Campbell wrote:
> On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>
> Would an "if (rc) libxl_cpupoolinfo_dispose(info)" on the exit path be a
> more robust alternative? Might require the addition of a
> libxl_cpupoolinfo_init() somewhere before any possible error.
>
Yes, you're right, it would be better to fix the caller.
This results in a larger patch than this one. I will send out the new
version soon.
Wei.
> > ---
> > tools/libxl/libxl.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 38aff8d..4151dcb 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -740,8 +740,10 @@ static int cpupool_info(libxl__gc *gc,
> > info->sched = xcinfo->sched_id;
> > info->n_dom = xcinfo->n_dom;
> > rc = libxl_cpu_bitmap_alloc(CTX, &info->cpumap, 0);
> > - if (rc)
> > + if (rc) {
> > + free(info->pool_name);
> > goto out;
> > + }
> >
> > memcpy(info->cpumap.map, xcinfo->cpumap, info->cpumap.size);
> >
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 5/9] libxl: avoid leaking string in cpupool_info
2015-07-13 16:10 ` Wei Liu
@ 2015-07-13 16:23 ` Ian Campbell
2015-07-13 16:24 ` Wei Liu
0 siblings, 1 reply; 34+ messages in thread
From: Ian Campbell @ 2015-07-13 16:23 UTC (permalink / raw)
To: Wei Liu; +Cc: Xen-devel, Ian Jackson, Andrew Cooper
On Mon, 2015-07-13 at 17:10 +0100, Wei Liu wrote:
> On Mon, Jul 13, 2015 at 11:07:31AM +0100, Ian Campbell wrote:
> > On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> >
> > Would an "if (rc) libxl_cpupoolinfo_dispose(info)" on the exit path be a
> > more robust alternative? Might require the addition of a
> > libxl_cpupoolinfo_init() somewhere before any possible error.
> >
>
> Yes, you're right, it would be better to fix the caller.
I was suggesting to do it in this function, not the caller.
>
> This results in a larger patch than this one. I will send out the new
> version soon.
>
> Wei.
>
> > > ---
> > > tools/libxl/libxl.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > index 38aff8d..4151dcb 100644
> > > --- a/tools/libxl/libxl.c
> > > +++ b/tools/libxl/libxl.c
> > > @@ -740,8 +740,10 @@ static int cpupool_info(libxl__gc *gc,
> > > info->sched = xcinfo->sched_id;
> > > info->n_dom = xcinfo->n_dom;
> > > rc = libxl_cpu_bitmap_alloc(CTX, &info->cpumap, 0);
> > > - if (rc)
> > > + if (rc) {
> > > + free(info->pool_name);
> > > goto out;
> > > + }
> > >
> > > memcpy(info->cpumap.map, xcinfo->cpumap, info->cpumap.size);
> > >
> >
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 5/9] libxl: avoid leaking string in cpupool_info
2015-07-13 16:23 ` Ian Campbell
@ 2015-07-13 16:24 ` Wei Liu
0 siblings, 0 replies; 34+ messages in thread
From: Wei Liu @ 2015-07-13 16:24 UTC (permalink / raw)
To: Ian Campbell; +Cc: Xen-devel, Wei Liu, Ian Jackson, Andrew Cooper
On Mon, Jul 13, 2015 at 05:23:10PM +0100, Ian Campbell wrote:
> On Mon, 2015-07-13 at 17:10 +0100, Wei Liu wrote:
> > On Mon, Jul 13, 2015 at 11:07:31AM +0100, Ian Campbell wrote:
> > > On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > >
> > > Would an "if (rc) libxl_cpupoolinfo_dispose(info)" on the exit path be a
> > > more robust alternative? Might require the addition of a
> > > libxl_cpupoolinfo_init() somewhere before any possible error.
> > >
> >
> > Yes, you're right, it would be better to fix the caller.
>
> I was suggesting to do it in this function, not the caller.
>
No, that's not right, that violates libxl calling style. The caller is
supposed to call _init function before coming to this function.
Wei.
>
> >
> > This results in a larger patch than this one. I will send out the new
> > version soon.
> >
> > Wei.
> >
> > > > ---
> > > > tools/libxl/libxl.c | 4 +++-
> > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > > index 38aff8d..4151dcb 100644
> > > > --- a/tools/libxl/libxl.c
> > > > +++ b/tools/libxl/libxl.c
> > > > @@ -740,8 +740,10 @@ static int cpupool_info(libxl__gc *gc,
> > > > info->sched = xcinfo->sched_id;
> > > > info->n_dom = xcinfo->n_dom;
> > > > rc = libxl_cpu_bitmap_alloc(CTX, &info->cpumap, 0);
> > > > - if (rc)
> > > > + if (rc) {
> > > > + free(info->pool_name);
> > > > goto out;
> > > > + }
> > > >
> > > > memcpy(info->cpumap.map, xcinfo->cpumap, info->cpumap.size);
> > > >
> > >
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/9] libxl: fix libxl__abs_path
2015-07-13 9:57 ` Ian Campbell
2015-07-13 10:00 ` Wei Liu
@ 2015-07-13 17:12 ` Ian Jackson
2015-07-14 7:23 ` Ian Campbell
1 sibling, 1 reply; 34+ messages in thread
From: Ian Jackson @ 2015-07-13 17:12 UTC (permalink / raw)
To: Ian Campbell; +Cc: Xen-devel, Wei Liu, Andrew Cooper
Ian Campbell writes ("Re: [PATCH 1/9] libxl: fix libxl__abs_path"):
> I rather dislike subjects of the form "fix $function", since it gives
> very little clue to someone reading the shortlog what is going on.
Yes.
> In this case I think "libxl: make libxl__abs_path correctly handle a
> NULL argument" would be an accurate description.
But: it is quite surprising that libxl__abs_path can be legally passed
a NULL for any of its parameters.
There are no call sites in libxl which can pass a NULL.
I think that if we are to retain this feature, it ought to be
documented, at least.
Maybe
_hidden char *libxl__abs_path(libxl__gc *gc,
const char *s /* NULL OK */,
const char *path);
?
Ian.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/9] libxl: json string object can be NULL
2015-07-13 10:02 ` Ian Campbell
@ 2015-07-13 17:16 ` Ian Jackson
0 siblings, 0 replies; 34+ messages in thread
From: Ian Jackson @ 2015-07-13 17:16 UTC (permalink / raw)
To: Ian Campbell; +Cc: Xen-devel, Wei Liu, Andrew Cooper
Ian Campbell writes ("Re: [PATCH 3/9] libxl: json string object can be NULL"):
> On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> It occurs to me that since libxl__strdup can never return NULL due to
> allocation failure we could consider make libxl__strdup(gc, NULL) be
> well defined as returning NULL.
This would be a nicer fix, yes.
Ian.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/9] libxl: dispose dominfo to avoid leaking resource
2015-07-13 10:05 ` Ian Campbell
@ 2015-07-13 17:18 ` Ian Jackson
0 siblings, 0 replies; 34+ messages in thread
From: Ian Jackson @ 2015-07-13 17:18 UTC (permalink / raw)
To: Ian Campbell; +Cc: Xen-devel, Wei Liu, Andrew Cooper
Ian Campbell writes ("Re: [PATCH 4/9] libxl: dispose dominfo to avoid leaking resource"):
> On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> > Add libxl_dominfo_dispose to one return path that doesn't have it.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>
> That return is a bit at odds with the generally correct error handling
> in that function, but this improves things at least a little and I can
> sort of see why this a slightly special case, so:
I think this should be done by changing `return' to `goto
out_success' or some such.
Ian.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/9] libxl: fix libxl__abs_path
2015-07-13 17:12 ` Ian Jackson
@ 2015-07-14 7:23 ` Ian Campbell
2015-07-14 10:23 ` Ian Jackson
0 siblings, 1 reply; 34+ messages in thread
From: Ian Campbell @ 2015-07-14 7:23 UTC (permalink / raw)
To: Ian Jackson; +Cc: Xen-devel, Wei Liu, Andrew Cooper
On Mon, 2015-07-13 at 18:12 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 1/9] libxl: fix libxl__abs_path"):
> > I rather dislike subjects of the form "fix $function", since it gives
> > very little clue to someone reading the shortlog what is going on.
>
> Yes.
>
> > In this case I think "libxl: make libxl__abs_path correctly handle a
> > NULL argument" would be an accurate description.
>
> But: it is quite surprising that libxl__abs_path can be legally passed
> a NULL for any of its parameters.
True.
>
> There are no call sites in libxl which can pass a NULL.
>
> I think that if we are to retain this feature, it ought to be
> documented, at least.
>
> Maybe
>
> _hidden char *libxl__abs_path(libxl__gc *gc,
> const char *s /* NULL OK */,
> const char *path);
Or add an assert if we don't wish to support this?
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/9] libxl: fix libxl__abs_path
2015-07-14 7:23 ` Ian Campbell
@ 2015-07-14 10:23 ` Ian Jackson
2015-07-14 13:32 ` Ian Campbell
2015-07-14 15:50 ` Wei Liu
0 siblings, 2 replies; 34+ messages in thread
From: Ian Jackson @ 2015-07-14 10:23 UTC (permalink / raw)
To: Ian Campbell; +Cc: Xen-devel, Wei Liu, Andrew Cooper
Ian Campbell writes ("Re: [PATCH 1/9] libxl: fix libxl__abs_path"):
> On Mon, 2015-07-13 at 18:12 +0100, Ian Jackson wrote:
> > There are no call sites in libxl which can pass a NULL.
> >
> > I think that if we are to retain this feature, it ought to be
> > documented, at least.
>
> Or add an assert if we don't wish to support this?
If we don't want to support it we can just remove the check and let it
crash.
Ian.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/9] libxl: fix libxl__abs_path
2015-07-14 10:23 ` Ian Jackson
@ 2015-07-14 13:32 ` Ian Campbell
2015-07-14 13:58 ` Ian Jackson
2015-07-14 15:50 ` Wei Liu
1 sibling, 1 reply; 34+ messages in thread
From: Ian Campbell @ 2015-07-14 13:32 UTC (permalink / raw)
To: Ian Jackson; +Cc: Xen-devel, Wei Liu, Andrew Cooper
On Tue, 2015-07-14 at 11:23 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 1/9] libxl: fix libxl__abs_path"):
> > On Mon, 2015-07-13 at 18:12 +0100, Ian Jackson wrote:
> > > There are no call sites in libxl which can pass a NULL.
> > >
> > > I think that if we are to retain this feature, it ought to be
> > > documented, at least.
> >
> > Or add an assert if we don't wish to support this?
>
> If we don't want to support it we can just remove the check and let it
> crash.
An assert at least gets us a slightly improved message when it happens.
Ian.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/9] libxl: fix libxl__abs_path
2015-07-14 13:32 ` Ian Campbell
@ 2015-07-14 13:58 ` Ian Jackson
2015-07-14 14:10 ` Ian Campbell
0 siblings, 1 reply; 34+ messages in thread
From: Ian Jackson @ 2015-07-14 13:58 UTC (permalink / raw)
To: Ian Campbell; +Cc: Xen-devel, Wei Liu, Andrew Cooper
Ian Campbell writes ("Re: [PATCH 1/9] libxl: fix libxl__abs_path"):
> On Tue, 2015-07-14 at 11:23 +0100, Ian Jackson wrote:
> > If we don't want to support it we can just remove the check and let it
> > crash.
>
> An assert at least gets us a slightly improved message when it happens.
If that logic applies here it applies to almost every function in
libxl. I don't think this is a sensible approach.
Ian.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/9] libxl: fix libxl__abs_path
2015-07-14 13:58 ` Ian Jackson
@ 2015-07-14 14:10 ` Ian Campbell
0 siblings, 0 replies; 34+ messages in thread
From: Ian Campbell @ 2015-07-14 14:10 UTC (permalink / raw)
To: Ian Jackson; +Cc: Xen-devel, Wei Liu, Andrew Cooper
On Tue, 2015-07-14 at 14:58 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 1/9] libxl: fix libxl__abs_path"):
> > On Tue, 2015-07-14 at 11:23 +0100, Ian Jackson wrote:
> > > If we don't want to support it we can just remove the check and let it
> > > crash.
> >
> > An assert at least gets us a slightly improved message when it happens.
>
> If that logic applies here it applies to almost every function in
> libxl. I don't think this is a sensible approach.
OK.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/9] libxl: fix libxl__abs_path
2015-07-14 10:23 ` Ian Jackson
2015-07-14 13:32 ` Ian Campbell
@ 2015-07-14 15:50 ` Wei Liu
1 sibling, 0 replies; 34+ messages in thread
From: Wei Liu @ 2015-07-14 15:50 UTC (permalink / raw)
To: Ian Jackson; +Cc: Xen-devel, Wei Liu, Ian Campbell, Andrew Cooper
On Tue, Jul 14, 2015 at 11:23:27AM +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 1/9] libxl: fix libxl__abs_path"):
> > On Mon, 2015-07-13 at 18:12 +0100, Ian Jackson wrote:
> > > There are no call sites in libxl which can pass a NULL.
> > >
> > > I think that if we are to retain this feature, it ought to be
> > > documented, at least.
> >
> > Or add an assert if we don't wish to support this?
>
> If we don't want to support it we can just remove the check and let it
> crash.
>
I will drop this patch and create a new one with this approach.
Wei.
> Ian.
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2015-07-14 15:51 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-10 18:00 [PATCH 0/9] xl/libxl: fix issues discovered by Coverity scan Wei Liu
2015-07-10 18:00 ` [PATCH 1/9] libxl: fix libxl__abs_path Wei Liu
2015-07-13 9:57 ` Ian Campbell
2015-07-13 10:00 ` Wei Liu
2015-07-13 17:12 ` Ian Jackson
2015-07-14 7:23 ` Ian Campbell
2015-07-14 10:23 ` Ian Jackson
2015-07-14 13:32 ` Ian Campbell
2015-07-14 13:58 ` Ian Jackson
2015-07-14 14:10 ` Ian Campbell
2015-07-14 15:50 ` Wei Liu
2015-07-10 18:00 ` [PATCH 2/9] libxl: turn two malloc's to libxl__malloc Wei Liu
2015-07-13 10:00 ` Ian Campbell
2015-07-13 15:29 ` Wei Liu
2015-07-10 18:00 ` [PATCH 3/9] libxl: json string object can be NULL Wei Liu
2015-07-13 10:02 ` Ian Campbell
2015-07-13 17:16 ` Ian Jackson
2015-07-10 18:00 ` [PATCH 4/9] libxl: dispose dominfo to avoid leaking resource Wei Liu
2015-07-13 10:05 ` Ian Campbell
2015-07-13 17:18 ` Ian Jackson
2015-07-10 18:00 ` [PATCH 5/9] libxl: avoid leaking string in cpupool_info Wei Liu
2015-07-13 10:07 ` Ian Campbell
2015-07-13 16:10 ` Wei Liu
2015-07-13 16:23 ` Ian Campbell
2015-07-13 16:24 ` Wei Liu
2015-07-10 18:00 ` [PATCH 6/9] libxl: localtime(3) can return NULL Wei Liu
2015-07-13 10:09 ` Ian Campbell
2015-07-10 18:00 ` [PATCH 7/9] libxl: qmp_init_handler " Wei Liu
2015-07-13 10:10 ` Ian Campbell
2015-07-10 18:00 ` [PATCH 8/9] xl: fix main_cpupoolcreate Wei Liu
2015-07-13 10:11 ` Ian Campbell
2015-07-10 18:00 ` [PATCH 9/9] xl: fix main_config_update Wei Liu
2015-07-13 10:12 ` Ian Campbell
2015-07-10 18:21 ` [PATCH 0/9] xl/libxl: fix issues discovered by Coverity scan Andrew Cooper
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).