[libvirt] [PATCH] lxc: avoid missing '{' in the function
From: Alex Jia <ajia@redhat.com> Cppcheck detected a syntaxError on lxcDomainInterfaceStats. * src/lxc/lxc_driver.c: fixed missing '{' in the function lxcDomainInterfaceStats. Signed-off-by: Alex Jia <ajia@redhat.com> --- src/lxc/lxc_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index f08e8d1..06bfa85 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2671,6 +2671,7 @@ static int lxcDomainInterfaceStats(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED, struct _virDomainInterfaceStats *stats ATTRIBUTE_UNUSED) +{ lxcError(VIR_ERR_NO_SUPPORT, "%s", __FUNCTION__); return -1; } -- 1.7.1
From: Alex Jia <ajia@redhat.com> If the function lxcSetupLoopDevices(def, &nloopDevs, &loopDevs) failed, the variable loopDevs will keep a initial NULL value, however, the function VIR_FORCE_CLOSE(loopDevs[i]) will directly deref it. * rc/lxc/lxc_controller.c: fixed a null pointer dereference. Signed-off-by: Alex Jia <ajia@redhat.com> --- src/lxc/lxc_controller.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index c4e7832..024756d 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1017,8 +1017,11 @@ cleanup: VIR_FORCE_CLOSE(containerhandshake[0]); VIR_FORCE_CLOSE(containerhandshake[1]); - for (i = 0 ; i < nloopDevs ; i++) - VIR_FORCE_CLOSE(loopDevs[i]); + if (loopDevs) { + for (i = 0 ; i < nloopDevs ; i++) + VIR_FORCE_CLOSE(loopDevs[i]); + } + VIR_FREE(loopDevs); if (container > 1) { -- 1.7.1
On 10/27/2011 09:18 AM, ajia@redhat.com wrote:
From: Alex Jia<ajia@redhat.com>
If the function lxcSetupLoopDevices(def,&nloopDevs,&loopDevs) failed, the variable loopDevs will keep a initial NULL value, however, the function VIR_FORCE_CLOSE(loopDevs[i]) will directly deref it.
* rc/lxc/lxc_controller.c: fixed a null pointer dereference.
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/lxc/lxc_controller.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index c4e7832..024756d 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1017,8 +1017,11 @@ cleanup: VIR_FORCE_CLOSE(containerhandshake[0]); VIR_FORCE_CLOSE(containerhandshake[1]);
- for (i = 0 ; i< nloopDevs ; i++) - VIR_FORCE_CLOSE(loopDevs[i]);
Indeed, this situation might happen if memory reallocation fails after some iterations of the loop inside of lxcSetupLoopDevices, leaving nloopDevs assigned to some value, but loopDevs being NULL.
+ if (loopDevs) { + for (i = 0 ; i< nloopDevs ; i++) + VIR_FORCE_CLOSE(loopDevs[i]); + } + VIR_FREE(loopDevs);
if (container> 1) {
ACK. I squashed in a fix for seting the device counter to 0 if this happens. (Well it will be fixed on two places at once, as lxcSetupLoopDevices is called only from here). diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 024756d..7603bc7 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -208,6 +208,7 @@ static int lxcSetupLoopDevices(virDomainDefPtr def, size_t *nloopDevs, int **loo VIR_DEBUG("Saving loop fd %d", fd); if (VIR_REALLOC_N(*loopDevs, *nloopDevs+1) < 0) { + *nloopDevs = 0; VIR_FORCE_CLOSE(fd); virReportOOMError(); goto cleanup; and pushed. Peter
On Thu, Oct 27, 2011 at 10:06:09AM +0200, Peter Krempa wrote:
On 10/27/2011 09:18 AM, ajia@redhat.com wrote:
From: Alex Jia<ajia@redhat.com>
If the function lxcSetupLoopDevices(def,&nloopDevs,&loopDevs) failed, the variable loopDevs will keep a initial NULL value, however, the function VIR_FORCE_CLOSE(loopDevs[i]) will directly deref it.
* rc/lxc/lxc_controller.c: fixed a null pointer dereference.
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/lxc/lxc_controller.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index c4e7832..024756d 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1017,8 +1017,11 @@ cleanup: VIR_FORCE_CLOSE(containerhandshake[0]); VIR_FORCE_CLOSE(containerhandshake[1]);
- for (i = 0 ; i< nloopDevs ; i++) - VIR_FORCE_CLOSE(loopDevs[i]);
Indeed, this situation might happen if memory reallocation fails after some iterations of the loop inside of lxcSetupLoopDevices, leaving nloopDevs assigned to some value, but loopDevs being NULL.
No it can't. If VIR_REALLOC_N fails, it guarentees that the original pointer is left at its original value. It can't become NULL. This is critical, because we need to release any FDs that were stored into loopDevs in earlier iterations of the loop
+ if (loopDevs) { + for (i = 0 ; i< nloopDevs ; i++) + VIR_FORCE_CLOSE(loopDevs[i]); + } + VIR_FREE(loopDevs);
if (container> 1) {
ACK. I squashed in a fix for seting the device counter to 0 if this happens. (Well it will be fixed on two places at once, as lxcSetupLoopDevices is called only from here).
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 024756d..7603bc7 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -208,6 +208,7 @@ static int lxcSetupLoopDevices(virDomainDefPtr def, size_t *nloopDevs, int **loo
VIR_DEBUG("Saving loop fd %d", fd); if (VIR_REALLOC_N(*loopDevs, *nloopDevs+1) < 0) { + *nloopDevs = 0; VIR_FORCE_CLOSE(fd); virReportOOMError(); goto cleanup;
and pushed.
This is actually *wrong*. You now leak any file descriptors that were stored in loopDevs prior to the VIR_REALLOC_N failure. Please revert this hunk Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/27/2011 10:18 AM, Daniel P. Berrange wrote:
On Thu, Oct 27, 2011 at 10:06:09AM +0200, Peter Krempa wrote:
On 10/27/2011 09:18 AM, ajia@redhat.com wrote:
From: Alex Jia<ajia@redhat.com>
Indeed, this situation might happen if memory reallocation fails after some iterations of the loop inside of lxcSetupLoopDevices, leaving nloopDevs assigned to some value, but loopDevs being NULL.
No it can't. If VIR_REALLOC_N fails, it guarentees that the original pointer is left at its original value. It can't become NULL. This is critical, because we need to release any FDs that were stored into loopDevs in earlier iterations of the loop
Uh :/
+ if (loopDevs) { + for (i = 0 ; i< nloopDevs ; i++) + VIR_FORCE_CLOSE(loopDevs[i]);
and pushed.
This is actually *wrong*. You now leak any file descriptors that were stored in loopDevs prior to the VIR_REALLOC_N failure. Please revert this hunk
Yes, if the pointer is retained, it'll definitely leak the FD's :(. I'm sorry for not noticing that. :( Reverted with: commit 95d3b4de714049e4b6b2033e2db9151ae11d6742 Author: Peter Krempa <pkrempa@redhat.com> Date: Thu Oct 27 10:24:30 2011 +0200 lxc: Revert zeroing count of allocated items if VIR_REALLOC_N fails Previous commit clears number of items alocated in lxcSetupLoopDevices if VIR_REALLOC_N fails. In that case, the pointer is not NULL, and causes leaking FDs that have been allocated. * src/lxc/lxc_controller.c: revert zeroing array size diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 7603bc7..024756d 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -208,7 +208,6 @@ static int lxcSetupLoopDevices(virDomainDefPtr def, size_t *nloopDevs, int **loo VIR_DEBUG("Saving loop fd %d", fd); if (VIR_REALLOC_N(*loopDevs, *nloopDevs+1) < 0) { - *nloopDevs = 0; VIR_FORCE_CLOSE(fd); virReportOOMError(); goto cleanup; Peter
Daniel
On Thu, Oct 27, 2011 at 10:34:36AM +0200, Peter Krempa wrote:
On 10/27/2011 10:18 AM, Daniel P. Berrange wrote:
On Thu, Oct 27, 2011 at 10:06:09AM +0200, Peter Krempa wrote:
On 10/27/2011 09:18 AM, ajia@redhat.com wrote:
From: Alex Jia<ajia@redhat.com>
Indeed, this situation might happen if memory reallocation fails after some iterations of the loop inside of lxcSetupLoopDevices, leaving nloopDevs assigned to some value, but loopDevs being NULL.
No it can't. If VIR_REALLOC_N fails, it guarentees that the original pointer is left at its original value. It can't become NULL. This is critical, because we need to release any FDs that were stored into loopDevs in earlier iterations of the loop
Uh :/
The retention of the original pointer is in fact one of the primary reasons why we invented the VIR_REALLOC_N macro, as a replacement for realloc(). If you're interested, I describe the design in some more detail here http://berrange.com/posts/2008/05/23/better-living-through-api-design-low-le...
+ if (loopDevs) { + for (i = 0 ; i< nloopDevs ; i++) + VIR_FORCE_CLOSE(loopDevs[i]);
and pushed.
This is actually *wrong*. You now leak any file descriptors that were stored in loopDevs prior to the VIR_REALLOC_N failure. Please revert this hunk
Yes, if the pointer is retained, it'll definitely leak the FD's :(.
I'm sorry for not noticing that. :(
Reverted with:
commit 95d3b4de714049e4b6b2033e2db9151ae11d6742 Author: Peter Krempa <pkrempa@redhat.com> Date: Thu Oct 27 10:24:30 2011 +0200
lxc: Revert zeroing count of allocated items if VIR_REALLOC_N fails
Previous commit clears number of items alocated in lxcSetupLoopDevices if VIR_REALLOC_N fails. In that case, the pointer is not NULL, and causes leaking FDs that have been allocated.
* src/lxc/lxc_controller.c: revert zeroing array size
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 7603bc7..024756d 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -208,7 +208,6 @@ static int lxcSetupLoopDevices(virDomainDefPtr def, size_t *nloopDevs, int **loo
VIR_DEBUG("Saving loop fd %d", fd); if (VIR_REALLOC_N(*loopDevs, *nloopDevs+1) < 0) { - *nloopDevs = 0; VIR_FORCE_CLOSE(fd); virReportOOMError(); goto cleanup;
Thanks for pushing this quickly ! Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
From: Alex Jia <ajia@redhat.com> Detected by cppcheck, the previous function VIR_REALLOC_N(mounts, nmounts+1) can make sure mounts is a none null value, so it is redundant to check if mounts is null. * src/lxc/lxc_container.c: remove redundant check. Signed-off-by: Alex Jia <ajia@redhat.com> --- src/lxc/lxc_container.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 06ccf7e..f4879c3 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -879,9 +879,8 @@ static int lxcContainerUnmountOldFS(void) } endmntent(procmnt); - if (mounts) - qsort(mounts, nmounts, sizeof(mounts[0]), - lxcContainerChildMountSort); + qsort(mounts, nmounts, sizeof(mounts[0]), + lxcContainerChildMountSort); for (i = 0 ; i < nmounts ; i++) { VIR_DEBUG("Umount %s", mounts[i]); -- 1.7.1
On Thu, Oct 27, 2011 at 03:18:01PM +0800, ajia@redhat.com wrote:
From: Alex Jia <ajia@redhat.com>
Detected by cppcheck, the previous function VIR_REALLOC_N(mounts, nmounts+1) can make sure mounts is a none null value, so it is redundant to check if mounts is null.
VIR_REALLOC_N is only executed inside the loop. Although it is unlikely, in theory there can be zero iterations of the loop, in which case 'mounts' will be NULL at the point qsort is called.
* src/lxc/lxc_container.c: remove redundant check.
Signed-off-by: Alex Jia <ajia@redhat.com> --- src/lxc/lxc_container.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 06ccf7e..f4879c3 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -879,9 +879,8 @@ static int lxcContainerUnmountOldFS(void) } endmntent(procmnt);
- if (mounts) - qsort(mounts, nmounts, sizeof(mounts[0]), - lxcContainerChildMountSort); + qsort(mounts, nmounts, sizeof(mounts[0]), + lxcContainerChildMountSort);
for (i = 0 ; i < nmounts ; i++) { VIR_DEBUG("Umount %s", mounts[i]);
NACK, the check for NULL is correct Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/27/2011 04:20 PM, Daniel P. Berrange wrote:
On Thu, Oct 27, 2011 at 03:18:01PM +0800, ajia@redhat.com wrote:
From: Alex Jia<ajia@redhat.com>
Detected by cppcheck, the previous function VIR_REALLOC_N(mounts, nmounts+1) can make sure mounts is a none null value, so it is redundant to check if mounts is null. VIR_REALLOC_N is only executed inside the loop. Although it is unlikely, in theory there can be zero iterations of the loop, in which case 'mounts' will be NULL at the point qsort is called.
* src/lxc/lxc_container.c: remove redundant check.
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/lxc/lxc_container.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 06ccf7e..f4879c3 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -879,9 +879,8 @@ static int lxcContainerUnmountOldFS(void) } endmntent(procmnt);
- if (mounts) - qsort(mounts, nmounts, sizeof(mounts[0]), - lxcContainerChildMountSort); + qsort(mounts, nmounts, sizeof(mounts[0]), + lxcContainerChildMountSort);
for (i = 0 ; i< nmounts ; i++) { VIR_DEBUG("Umount %s", mounts[i]); NACK, the check for NULL is correct
Daniel Hi Daniel, My comment is wrong, but the following judgement should make sure mounts is non null: <snip> if (!(mounts[nmounts++] = strdup(mntent.mnt_dir))) { endmntent(procmnt); virReportOOMError(); return -1; } </snip>
Thanks for your review, Alex
On Thu, Oct 27, 2011 at 04:33:50PM +0800, Alex Jia wrote:
On 10/27/2011 04:20 PM, Daniel P. Berrange wrote:
On Thu, Oct 27, 2011 at 03:18:01PM +0800, ajia@redhat.com wrote:
From: Alex Jia<ajia@redhat.com>
Detected by cppcheck, the previous function VIR_REALLOC_N(mounts, nmounts+1) can make sure mounts is a none null value, so it is redundant to check if mounts is null. VIR_REALLOC_N is only executed inside the loop. Although it is unlikely, in theory there can be zero iterations of the loop, in which case 'mounts' will be NULL at the point qsort is called.
* src/lxc/lxc_container.c: remove redundant check.
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/lxc/lxc_container.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 06ccf7e..f4879c3 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -879,9 +879,8 @@ static int lxcContainerUnmountOldFS(void) } endmntent(procmnt);
- if (mounts) - qsort(mounts, nmounts, sizeof(mounts[0]), - lxcContainerChildMountSort); + qsort(mounts, nmounts, sizeof(mounts[0]), + lxcContainerChildMountSort);
for (i = 0 ; i< nmounts ; i++) { VIR_DEBUG("Umount %s", mounts[i]); NACK, the check for NULL is correct
Daniel Hi Daniel, My comment is wrong, but the following judgement should make sure mounts is non null: <snip> if (!(mounts[nmounts++] = strdup(mntent.mnt_dir))) { endmntent(procmnt); virReportOOMError(); return -1; } </snip>
That is ensuring that the pointer in the array element value is non-NULL. The "if (mounts) qsort(...)" check is against the array itself, rather than the element. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Oct 27, 2011 at 04:33:50PM +0800, Alex Jia wrote:
On 10/27/2011 04:20 PM, Daniel P. Berrange wrote:
On Thu, Oct 27, 2011 at 03:18:01PM +0800, ajia@redhat.com wrote:
From: Alex Jia<ajia@redhat.com>
Detected by cppcheck, the previous function VIR_REALLOC_N(mounts, nmounts+1) can make sure mounts is a none null value, so it is redundant to check if mounts is null. VIR_REALLOC_N is only executed inside the loop. Although it is unlikely, in theory there can be zero iterations of the loop, in which case 'mounts' will be NULL at the point qsort is called.
* src/lxc/lxc_container.c: remove redundant check.
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/lxc/lxc_container.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 06ccf7e..f4879c3 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -879,9 +879,8 @@ static int lxcContainerUnmountOldFS(void) } endmntent(procmnt);
- if (mounts) - qsort(mounts, nmounts, sizeof(mounts[0]), - lxcContainerChildMountSort); + qsort(mounts, nmounts, sizeof(mounts[0]), + lxcContainerChildMountSort);
for (i = 0 ; i< nmounts ; i++) { VIR_DEBUG("Umount %s", mounts[i]); NACK, the check for NULL is correct
Daniel Hi Daniel, My comment is wrong, but the following judgement should make sure mounts is non null: <snip> if (!(mounts[nmounts++] = strdup(mntent.mnt_dir))) { If the previous VIR_REALLOC_N can't make sure 'mounts' is non null, whether we need to check 'mounts' to avoid dereferencing a NULL
On 10/27/2011 05:26 PM, Daniel P. Berrange wrote: pointer in here again, for instance: if (!mounts || !(mounts[nmounts++] = strdup(mntent.mnt_dir))) { Thanks for your comment. Alex
endmntent(procmnt); virReportOOMError(); return -1; } </snip>
That is ensuring that the pointer in the array element value is non-NULL. The "if (mounts) qsort(...)" check is against the array itself, rather than the element.
Regards, Daniel
On Thu, Oct 27, 2011 at 05:54:20PM +0800, Alex Jia wrote:
On Thu, Oct 27, 2011 at 04:33:50PM +0800, Alex Jia wrote:
On 10/27/2011 04:20 PM, Daniel P. Berrange wrote:
On Thu, Oct 27, 2011 at 03:18:01PM +0800, ajia@redhat.com wrote:
From: Alex Jia<ajia@redhat.com>
Detected by cppcheck, the previous function VIR_REALLOC_N(mounts, nmounts+1) can make sure mounts is a none null value, so it is redundant to check if mounts is null. VIR_REALLOC_N is only executed inside the loop. Although it is unlikely, in theory there can be zero iterations of the loop, in which case 'mounts' will be NULL at the point qsort is called.
* src/lxc/lxc_container.c: remove redundant check.
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/lxc/lxc_container.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 06ccf7e..f4879c3 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -879,9 +879,8 @@ static int lxcContainerUnmountOldFS(void) } endmntent(procmnt);
- if (mounts) - qsort(mounts, nmounts, sizeof(mounts[0]), - lxcContainerChildMountSort); + qsort(mounts, nmounts, sizeof(mounts[0]), + lxcContainerChildMountSort);
for (i = 0 ; i< nmounts ; i++) { VIR_DEBUG("Umount %s", mounts[i]); NACK, the check for NULL is correct
Daniel Hi Daniel, My comment is wrong, but the following judgement should make sure mounts is non null: <snip> if (!(mounts[nmounts++] = strdup(mntent.mnt_dir))) { If the previous VIR_REALLOC_N can't make sure 'mounts' is non null, whether we need to check 'mounts' to avoid dereferencing a NULL
On 10/27/2011 05:26 PM, Daniel P. Berrange wrote: pointer in here again, for instance:
if (!mounts || !(mounts[nmounts++] = strdup(mntent.mnt_dir))) {
The check for VIR_REALLOC_N ensures that mounts is non-NULL *if* there is at least 1 iteration of the while() loop. The "if (mounts) qsort" code is *outside* the while() loop, and has to cope with the scenario where the while() loop had *zero* iterations. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/27/2011 05:59 PM, Daniel P. Berrange wrote:
On Thu, Oct 27, 2011 at 05:54:20PM +0800, Alex Jia wrote:
On Thu, Oct 27, 2011 at 04:33:50PM +0800, Alex Jia wrote:
On 10/27/2011 04:20 PM, Daniel P. Berrange wrote:
On Thu, Oct 27, 2011 at 03:18:01PM +0800, ajia@redhat.com wrote:
From: Alex Jia<ajia@redhat.com>
Detected by cppcheck, the previous function VIR_REALLOC_N(mounts, nmounts+1) can make sure mounts is a none null value, so it is redundant to check if mounts is null. VIR_REALLOC_N is only executed inside the loop. Although it is unlikely, in theory there can be zero iterations of the loop, in which case 'mounts' will be NULL at the point qsort is called.
* src/lxc/lxc_container.c: remove redundant check.
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/lxc/lxc_container.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 06ccf7e..f4879c3 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -879,9 +879,8 @@ static int lxcContainerUnmountOldFS(void) } endmntent(procmnt);
- if (mounts) - qsort(mounts, nmounts, sizeof(mounts[0]), - lxcContainerChildMountSort); + qsort(mounts, nmounts, sizeof(mounts[0]), + lxcContainerChildMountSort);
for (i = 0 ; i< nmounts ; i++) { VIR_DEBUG("Umount %s", mounts[i]); NACK, the check for NULL is correct
Daniel Hi Daniel, My comment is wrong, but the following judgement should make sure mounts is non null: <snip> if (!(mounts[nmounts++] = strdup(mntent.mnt_dir))) { If the previous VIR_REALLOC_N can't make sure 'mounts' is non null, whether we need to check 'mounts' to avoid dereferencing a NULL
On 10/27/2011 05:26 PM, Daniel P. Berrange wrote: pointer in here again, for instance:
if (!mounts || !(mounts[nmounts++] = strdup(mntent.mnt_dir))) { The check for VIR_REALLOC_N ensures that mounts is non-NULL *if* there is at least 1 iteration of the while() loop. The "if (mounts) qsort" code is *outside* the while() loop, and has to cope with the scenario where the while() loop had *zero* iterations. Yeah, you're right, thanks a lot.
Alex
Regards, Daniel
On 10/27/2011 05:59 PM, Daniel P. Berrange wrote:
On Thu, Oct 27, 2011 at 05:54:20PM +0800, Alex Jia wrote:
On Thu, Oct 27, 2011 at 04:33:50PM +0800, Alex Jia wrote:
On 10/27/2011 04:20 PM, Daniel P. Berrange wrote:
On Thu, Oct 27, 2011 at 03:18:01PM +0800, ajia@redhat.com wrote:
From: Alex Jia<ajia@redhat.com>
Detected by cppcheck, the previous function VIR_REALLOC_N(mounts, nmounts+1) can make sure mounts is a none null value, so it is redundant to check if mounts is null. VIR_REALLOC_N is only executed inside the loop. Although it is unlikely, in theory there can be zero iterations of the loop, in which case 'mounts' will be NULL at the point qsort is called.
* src/lxc/lxc_container.c: remove redundant check.
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/lxc/lxc_container.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 06ccf7e..f4879c3 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -879,9 +879,8 @@ static int lxcContainerUnmountOldFS(void) } endmntent(procmnt);
- if (mounts) - qsort(mounts, nmounts, sizeof(mounts[0]), - lxcContainerChildMountSort); + qsort(mounts, nmounts, sizeof(mounts[0]), + lxcContainerChildMountSort);
for (i = 0 ; i< nmounts ; i++) { VIR_DEBUG("Umount %s", mounts[i]); NACK, the check for NULL is correct
Daniel Hi Daniel, My comment is wrong, but the following judgement should make sure mounts is non null: <snip> if (!(mounts[nmounts++] = strdup(mntent.mnt_dir))) { If the previous VIR_REALLOC_N can't make sure 'mounts' is non null, whether we need to check 'mounts' to avoid dereferencing a NULL
On 10/27/2011 05:26 PM, Daniel P. Berrange wrote: pointer in here again, for instance:
if (!mounts || !(mounts[nmounts++] = strdup(mntent.mnt_dir))) { The check for VIR_REALLOC_N ensures that mounts is non-NULL *if* there is at least 1 iteration of the while() loop. The "if (mounts) qsort" code is *outside* the while() loop, and has to cope with the scenario where the while() loop had *zero* iterations. Hmm, I just saw codes again, it indeed is a issue, however, I fixed a error place :-(
882 if (mounts) 883 qsort(mounts, nmounts, sizeof(mounts[0]), 884 lxcContainerChildMountSort); 885 *Notes: if mounts is NULL, the above 'if' clause can't be executed, but the following codes will be run.* 886 for (i = 0 ; i < nmounts ; i++) { 887 VIR_DEBUG("Umount %s", mounts[i]); *Notes: dereference a NULL pointer in here.* 888 if (umount(mounts[i]) < 0) { Notes: same as above. 889 virReportSystemError(errno, 890 _("Failed to unmount '%s'"), 891 mounts[i]); Notes: same as above. 892 return -1; 893 } 894 VIR_FREE(mounts[i]); Notes: same as above. 895 } 896 VIR_FREE(mounts); 897 898 return 0; 899 } So we should use a '{}' to surround with 883: qsort...894: VIR_FREE(mounts[i]); Daniel, please correct me if I'm wrong again :-) Thanks a lot! Alex
Regards, Daniel
On 27.10.2011 12:39, Alex Jia wrote:
On 10/27/2011 05:59 PM, Daniel P. Berrange wrote:
On Thu, Oct 27, 2011 at 05:54:20PM +0800, Alex Jia wrote:
On Thu, Oct 27, 2011 at 04:33:50PM +0800, Alex Jia wrote:
On 10/27/2011 04:20 PM, Daniel P. Berrange wrote:
On Thu, Oct 27, 2011 at 03:18:01PM +0800, ajia@redhat.com wrote: > From: Alex Jia<ajia@redhat.com> > > Detected by cppcheck, the previous function VIR_REALLOC_N(mounts, nmounts+1) > can make sure mounts is a none null value, so it is redundant to check if > mounts is null. VIR_REALLOC_N is only executed inside the loop. Although it is unlikely, in theory there can be zero iterations of the loop, in which case 'mounts' will be NULL at the point qsort is called.
> * src/lxc/lxc_container.c: remove redundant check. > > Signed-off-by: Alex Jia<ajia@redhat.com> > --- > src/lxc/lxc_container.c | 5 ++--- > 1 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c > index 06ccf7e..f4879c3 100644 > --- a/src/lxc/lxc_container.c > +++ b/src/lxc/lxc_container.c > @@ -879,9 +879,8 @@ static int lxcContainerUnmountOldFS(void) > } > endmntent(procmnt); > > - if (mounts) > - qsort(mounts, nmounts, sizeof(mounts[0]), > - lxcContainerChildMountSort); > + qsort(mounts, nmounts, sizeof(mounts[0]), > + lxcContainerChildMountSort); > > for (i = 0 ; i< nmounts ; i++) { > VIR_DEBUG("Umount %s", mounts[i]); NACK, the check for NULL is correct
Daniel Hi Daniel, My comment is wrong, but the following judgement should make sure mounts is non null: <snip> if (!(mounts[nmounts++] = strdup(mntent.mnt_dir))) { If the previous VIR_REALLOC_N can't make sure 'mounts' is non null, whether we need to check 'mounts' to avoid dereferencing a NULL
On 10/27/2011 05:26 PM, Daniel P. Berrange wrote: pointer in here again, for instance:
if (!mounts || !(mounts[nmounts++] = strdup(mntent.mnt_dir))) { The check for VIR_REALLOC_N ensures that mounts is non-NULL *if* there is at least 1 iteration of the while() loop. The "if (mounts) qsort" code is *outside* the while() loop, and has to cope with the scenario where the while() loop had *zero* iterations. Hmm, I just saw codes again, it indeed is a issue, however, I fixed a error place :-(
882 if (mounts) 883 qsort(mounts, nmounts, sizeof(mounts[0]), 884 lxcContainerChildMountSort); 885 *Notes: if mounts is NULL, the above 'if' clause can't be executed, but the following codes will be run.* 886 for (i = 0 ; i < nmounts ; i++) { 887 VIR_DEBUG("Umount %s", mounts[i]); *Notes: dereference a NULL pointer in here.*
The only way for nmounts being nonzero is mounts being non NULL; Thus, if mounts is NULL, there is no way for nmounts being something else than 0; Hence, the body of this for will not be executed. I think the code is good as-is. Michal
On 10/27/2011 06:46 PM, Michal Privoznik wrote:
On 27.10.2011 12:39, Alex Jia wrote:
On 10/27/2011 05:59 PM, Daniel P. Berrange wrote:
On Thu, Oct 27, 2011 at 05:54:20PM +0800, Alex Jia wrote:
On Thu, Oct 27, 2011 at 04:33:50PM +0800, Alex Jia wrote:
On 10/27/2011 04:20 PM, Daniel P. Berrange wrote: > On Thu, Oct 27, 2011 at 03:18:01PM +0800, ajia@redhat.com wrote: >> From: Alex Jia<ajia@redhat.com> >> >> Detected by cppcheck, the previous function VIR_REALLOC_N(mounts, nmounts+1) >> can make sure mounts is a none null value, so it is redundant to check if >> mounts is null. > VIR_REALLOC_N is only executed inside the loop. Although it is unlikely, > in theory there can be zero iterations of the loop, in which case > 'mounts' will be NULL at the point qsort is called. > >> * src/lxc/lxc_container.c: remove redundant check. >> >> Signed-off-by: Alex Jia<ajia@redhat.com> >> --- >> src/lxc/lxc_container.c | 5 ++--- >> 1 files changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c >> index 06ccf7e..f4879c3 100644 >> --- a/src/lxc/lxc_container.c >> +++ b/src/lxc/lxc_container.c >> @@ -879,9 +879,8 @@ static int lxcContainerUnmountOldFS(void) >> } >> endmntent(procmnt); >> >> - if (mounts) >> - qsort(mounts, nmounts, sizeof(mounts[0]), >> - lxcContainerChildMountSort); >> + qsort(mounts, nmounts, sizeof(mounts[0]), >> + lxcContainerChildMountSort); >> >> for (i = 0 ; i< nmounts ; i++) { >> VIR_DEBUG("Umount %s", mounts[i]); > NACK, the check for NULL is correct > > > Daniel Hi Daniel, My comment is wrong, but the following judgement should make sure mounts is non null: <snip> if (!(mounts[nmounts++] = strdup(mntent.mnt_dir))) { If the previous VIR_REALLOC_N can't make sure 'mounts' is non null, whether we need to check 'mounts' to avoid dereferencing a NULL
On 10/27/2011 05:26 PM, Daniel P. Berrange wrote: pointer in here again, for instance:
if (!mounts || !(mounts[nmounts++] = strdup(mntent.mnt_dir))) { The check for VIR_REALLOC_N ensures that mounts is non-NULL *if* there is at least 1 iteration of the while() loop. The "if (mounts) qsort" code is *outside* the while() loop, and has to cope with the scenario where the while() loop had *zero* iterations. Hmm, I just saw codes again, it indeed is a issue, however, I fixed a error place :-(
882 if (mounts) 883 qsort(mounts, nmounts, sizeof(mounts[0]), 884 lxcContainerChildMountSort); 885 *Notes: if mounts is NULL, the above 'if' clause can't be executed, but the following codes will be run.* 886 for (i = 0 ; i< nmounts ; i++) { 887 VIR_DEBUG("Umount %s", mounts[i]); *Notes: dereference a NULL pointer in here.* The only way for nmounts being nonzero is mounts being non NULL; Indeed, Michal, thanks for your comment :)
Alex
Thus, if mounts is NULL, there is no way for nmounts being something else than 0; Hence, the body of this for will not be executed.
I think the code is good as-is.
Michal
On Thu, Oct 27, 2011 at 06:39:57PM +0800, Alex Jia wrote:
On 10/27/2011 05:59 PM, Daniel P. Berrange wrote:
On Thu, Oct 27, 2011 at 05:54:20PM +0800, Alex Jia wrote:
On Thu, Oct 27, 2011 at 04:33:50PM +0800, Alex Jia wrote:
On 10/27/2011 04:20 PM, Daniel P. Berrange wrote:
On Thu, Oct 27, 2011 at 03:18:01PM +0800, ajia@redhat.com wrote: >From: Alex Jia<ajia@redhat.com> > >Detected by cppcheck, the previous function VIR_REALLOC_N(mounts, nmounts+1) >can make sure mounts is a none null value, so it is redundant to check if >mounts is null. VIR_REALLOC_N is only executed inside the loop. Although it is unlikely, in theory there can be zero iterations of the loop, in which case 'mounts' will be NULL at the point qsort is called.
>* src/lxc/lxc_container.c: remove redundant check. > >Signed-off-by: Alex Jia<ajia@redhat.com> >--- > src/lxc/lxc_container.c | 5 ++--- > 1 files changed, 2 insertions(+), 3 deletions(-) > >diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c >index 06ccf7e..f4879c3 100644 >--- a/src/lxc/lxc_container.c >+++ b/src/lxc/lxc_container.c >@@ -879,9 +879,8 @@ static int lxcContainerUnmountOldFS(void) > } > endmntent(procmnt); > >- if (mounts) >- qsort(mounts, nmounts, sizeof(mounts[0]), >- lxcContainerChildMountSort); >+ qsort(mounts, nmounts, sizeof(mounts[0]), >+ lxcContainerChildMountSort); > > for (i = 0 ; i< nmounts ; i++) { > VIR_DEBUG("Umount %s", mounts[i]); NACK, the check for NULL is correct
Daniel Hi Daniel, My comment is wrong, but the following judgement should make sure mounts is non null: <snip> if (!(mounts[nmounts++] = strdup(mntent.mnt_dir))) { If the previous VIR_REALLOC_N can't make sure 'mounts' is non null, whether we need to check 'mounts' to avoid dereferencing a NULL
On 10/27/2011 05:26 PM, Daniel P. Berrange wrote: pointer in here again, for instance:
if (!mounts || !(mounts[nmounts++] = strdup(mntent.mnt_dir))) { The check for VIR_REALLOC_N ensures that mounts is non-NULL *if* there is at least 1 iteration of the while() loop. The "if (mounts) qsort" code is *outside* the while() loop, and has to cope with the scenario where the while() loop had *zero* iterations. Hmm, I just saw codes again, it indeed is a issue, however, I fixed a error place :-(
882 if (mounts) 883 qsort(mounts, nmounts, sizeof(mounts[0]), 884 lxcContainerChildMountSort); 885 *Notes: if mounts is NULL, the above 'if' clause can't be executed, but the following codes will be run.* 886 for (i = 0 ; i < nmounts ; i++) { 887 VIR_DEBUG("Umount %s", mounts[i]); *Notes: dereference a NULL pointer in here.* 888 if (umount(mounts[i]) < 0) { Notes: same as above. 889 virReportSystemError(errno, 890 _("Failed to unmount '%s'"), 891 mounts[i]); Notes: same as above. 892 return -1; 893 } 894 VIR_FREE(mounts[i]); Notes: same as above. 895 } 896 VIR_FREE(mounts); 897 898 return 0; 899 }
So we should use a '{}' to surround with 883: qsort...894: VIR_FREE(mounts[i]);
There's no need. If nmounts > 0, then we know mounts != NULL. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/27/2011 04:54 AM, Daniel P. Berrange wrote:
882 if (mounts) 883 qsort(mounts, nmounts, sizeof(mounts[0]), 884 lxcContainerChildMountSort); 885 *Notes: if mounts is NULL, the above 'if' clause can't be executed, but the following codes will be run.* 886 for (i = 0 ; i< nmounts ; i++) {
If the static analyzer can't see that nmounts > 0 implies non-NULL mounts, then add right here: sa_assert(mounts);
887 VIR_DEBUG("Umount %s", mounts[i]);
So we should use a '{}' to surround with 883: qsort...894: VIR_FREE(mounts[i]);
There's no need. If nmounts> 0, then we know mounts != NULL.
Agree - the code is correct as-is; and it should take at most an sa_assert to shut up false positives in the analyzers. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 10/27/2011 09:17 AM, ajia@redhat.com wrote:
From: Alex Jia<ajia@redhat.com>
Cppcheck detected a syntaxError on lxcDomainInterfaceStats.
* src/lxc/lxc_driver.c: fixed missing '{' in the function lxcDomainInterfaceStats.
Signed-off-by: Alex Jia<ajia@redhat.com> ---
Indeed, it was in a conditionaly compiled section. ACK & pushed. Peter
On Thu, Oct 27, 2011 at 03:17:59PM +0800, ajia@redhat.com wrote:
From: Alex Jia <ajia@redhat.com>
Cppcheck detected a syntaxError on lxcDomainInterfaceStats.
* src/lxc/lxc_driver.c: fixed missing '{' in the function lxcDomainInterfaceStats.
Signed-off-by: Alex Jia <ajia@redhat.com> --- src/lxc/lxc_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index f08e8d1..06bfa85 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2671,6 +2671,7 @@ static int lxcDomainInterfaceStats(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED, struct _virDomainInterfaceStats *stats ATTRIBUTE_UNUSED) +{ lxcError(VIR_ERR_NO_SUPPORT, "%s", __FUNCTION__); return -1; }
Hmm, I dunno why we even have a '#ifdef __linux__' in that file since the entire file is Linux specific. We should just kill this #else clause entirely Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/27/2011 04:15 PM, Daniel P. Berrange wrote:
On Thu, Oct 27, 2011 at 03:17:59PM +0800, ajia@redhat.com wrote:
From: Alex Jia<ajia@redhat.com>
Cppcheck detected a syntaxError on lxcDomainInterfaceStats.
* src/lxc/lxc_driver.c: fixed missing '{' in the function lxcDomainInterfaceStats.
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/lxc/lxc_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index f08e8d1..06bfa85 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2671,6 +2671,7 @@ static int lxcDomainInterfaceStats(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED, struct _virDomainInterfaceStats *stats ATTRIBUTE_UNUSED) +{ lxcError(VIR_ERR_NO_SUPPORT, "%s", __FUNCTION__); return -1; } Hmm, I dunno why we even have a '#ifdef __linux__' in that file since the entire file is Linux specific. We should just kill this #else clause entirely You mean we should remove the following lines rather then reserving the function as a optional branch: #else static int lxcDomainInterfaceStats(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED, struct _virDomainInterfaceStats *stats ATTRIBUTE_UNUSED) lxcError(VIR_ERR_NO_SUPPORT, "%s", __FUNCTION__); return -1; }
If so, need I commit a v2 patch for this? Thanks for your review, Alex
Daniel
On Thu, Oct 27, 2011 at 04:51:05PM +0800, Alex Jia wrote:
On Thu, Oct 27, 2011 at 03:17:59PM +0800, ajia@redhat.com wrote:
From: Alex Jia<ajia@redhat.com>
Cppcheck detected a syntaxError on lxcDomainInterfaceStats.
* src/lxc/lxc_driver.c: fixed missing '{' in the function lxcDomainInterfaceStats.
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/lxc/lxc_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index f08e8d1..06bfa85 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2671,6 +2671,7 @@ static int lxcDomainInterfaceStats(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED, struct _virDomainInterfaceStats *stats ATTRIBUTE_UNUSED) +{ lxcError(VIR_ERR_NO_SUPPORT, "%s", __FUNCTION__); return -1; } Hmm, I dunno why we even have a '#ifdef __linux__' in that file since the entire file is Linux specific. We should just kill this #else clause entirely You mean we should remove the following lines rather then reserving
On 10/27/2011 04:15 PM, Daniel P. Berrange wrote: the function as a optional branch: #else static int lxcDomainInterfaceStats(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED, struct _virDomainInterfaceStats *stats ATTRIBUTE_UNUSED) lxcError(VIR_ERR_NO_SUPPORT, "%s", __FUNCTION__); return -1; }
If so, need I commit a v2 patch for this?
Yes, just remove that dummy function entirely. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/27/2011 05:26 PM, Daniel P. Berrange wrote:
On Thu, Oct 27, 2011 at 04:51:05PM +0800, Alex Jia wrote:
On Thu, Oct 27, 2011 at 03:17:59PM +0800, ajia@redhat.com wrote:
From: Alex Jia<ajia@redhat.com>
Cppcheck detected a syntaxError on lxcDomainInterfaceStats.
* src/lxc/lxc_driver.c: fixed missing '{' in the function lxcDomainInterfaceStats.
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/lxc/lxc_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index f08e8d1..06bfa85 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2671,6 +2671,7 @@ static int lxcDomainInterfaceStats(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED, struct _virDomainInterfaceStats *stats ATTRIBUTE_UNUSED) +{ lxcError(VIR_ERR_NO_SUPPORT, "%s", __FUNCTION__); return -1; } Hmm, I dunno why we even have a '#ifdef __linux__' in that file since the entire file is Linux specific. We should just kill this #else clause entirely You mean we should remove the following lines rather then reserving
On 10/27/2011 04:15 PM, Daniel P. Berrange wrote: the function as a optional branch: #else static int lxcDomainInterfaceStats(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED, struct _virDomainInterfaceStats *stats ATTRIBUTE_UNUSED) lxcError(VIR_ERR_NO_SUPPORT, "%s", __FUNCTION__); return -1; }
If so, need I commit a v2 patch for this? Yes, just remove that dummy function entirely.
Regards, Daniel Okay, I will commit a new patch for this instead of v2, because the patch summary isn't appropriate.
Thanks, Alex
participants (6)
-
ajia＠redhat.com -
Alex Jia
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik -
Peter Krempa