5:29 p.m.
From: Stefan Berger <stefanb(a)linux.vnet.ibm.com>
Avoid the freeing of an array of zero file descriptors in case
of error. Introduce a macro VIR_INIT_N_FD to initialize such
an array's elements to -1.
Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com>
---
src/qemu/qemu_hotplug.c | 14 +++++++++++---
src/util/virfile.h | 12 ++++++++++++
2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 6703c92..b295db2 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -874,9 +874,12 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
tapfdSize = vhostfdSize = net->driver.virtio.queues;
if (!tapfdSize)
tapfdSize = vhostfdSize = 1;
- if (VIR_ALLOC_N(tapfd, tapfdSize) < 0 ||
- VIR_ALLOC_N(vhostfd, vhostfdSize) < 0)
+ if (VIR_ALLOC_N(tapfd, tapfdSize) < 0)
goto cleanup;
+ VIR_INIT_N_FD(tapfd, tapfdSize);
+ if (VIR_ALLOC_N(vhostfd, vhostfdSize) < 0)
+ goto cleanup;
+ VIR_INIT_N_FD(vhostfd, vhostfdSize);
if (qemuNetworkIfaceConnect(vm->def, conn, driver, net,
priv->qemuCaps, tapfd, &tapfdSize) < 0)
goto cleanup;
@@ -885,8 +888,12 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
goto cleanup;
} else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
tapfdSize = vhostfdSize = 1;
- if (VIR_ALLOC(tapfd) < 0 || VIR_ALLOC(vhostfd) < 0)
+ if (VIR_ALLOC(tapfd) < 0)
+ goto cleanup;
+ *tapfd = -1;
+ if (VIR_ALLOC(vhostfd) < 0)
goto cleanup;
+ *vhostfd = -1;
if ((tapfd[0] = qemuPhysIfaceConnect(vm->def, driver, net,
priv->qemuCaps,
VIR_NETDEV_VPORT_PROFILE_OP_CREATE)) <
0)
@@ -898,6 +905,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
vhostfdSize = 1;
if (VIR_ALLOC(vhostfd) < 0)
goto cleanup;
+ *vhostfd = -1;
if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd,
&vhostfdSize) < 0)
goto cleanup;
}
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 20baf6f..802cf01 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -75,6 +75,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode)
ATTRIBUTE_RETURN_CHECK;
VIR_FILE_CLOSE_PRESERVE_ERRNO | \
VIR_FILE_CLOSE_DONT_LOG))
+static inline void vir_init_n_int(int *ptr, int count, int value)
+{
+ int i;
+
+ for (i = 0; i < count; i++)
+ ptr[i] = value;
+}
+
+/* Initialize an array of file descriptors to -1 */
+# define VIR_INIT_N_FD(ptr, count) \
+ vir_init_n_int(ptr, count, -1)
+
/* Opaque type for managing a wrapper around a fd. */
struct _virFileWrapperFd;
--
1.8.1.4
1:09 p.m.
On 03/08/2014 04:29 PM, Stefan Berger wrote:
From: Stefan Berger <stefanb(a)linux.vnet.ibm.com>
Avoid the freeing of an array of zero file descriptors in case
of error. Introduce a macro VIR_INIT_N_FD to initialize such
an array's elements to -1.
Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com>
---
src/qemu/qemu_hotplug.c | 14 +++++++++++---
src/util/virfile.h | 12 ++++++++++++
2 files changed, 23 insertions(+), 3 deletions(-)
+++ b/src/util/virfile.h
@@ -75,6 +75,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode)
ATTRIBUTE_RETURN_CHECK;
VIR_FILE_CLOSE_PRESERVE_ERRNO | \
VIR_FILE_CLOSE_DONT_LOG))
+static inline void vir_init_n_int(int *ptr, int count, int value)
+{
+ int i;
+
+ for (i = 0; i < count; i++)
+ ptr[i] = value;
+}
Do we ever plan on using this for values other than '-1'?
+
+/* Initialize an array of file descriptors to -1 */
+# define VIR_INIT_N_FD(ptr, count) \
+ vir_init_n_int(ptr, count, -1)
Could also be spelled:
memset(ptr, -1, sizeof(*ptr) * count))
which goes back to why we need vir_init_n_int().
I agree that the fix to qemu_hotplug.c to not close fd 0 on failure is
needed, but am not sure about the complexity of the virfile.h addition.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:10 p.m.
Eric Blake <eblake(a)redhat.com> wrote on 03/10/2014 02:09:58 PM:
From: Eric Blake <eblake(a)redhat.com>
To: Stefan Berger/Watson/IBM@IBMUS, libvir-list(a)redhat.com,
Cc: laine(a)laine.org
Date: 03/10/2014 02:24 PM
Subject: Re: [libvirt] [PATCH] BZ1072677: Avoid freeing of 0 file
descriptor
On 03/08/2014 04:29 PM, Stefan Berger wrote:
> From: Stefan Berger <stefanb(a)linux.vnet.ibm.com>
>
> Avoid the freeing of an array of zero file descriptors in case
> of error. Introduce a macro VIR_INIT_N_FD to initialize such
> an array's elements to -1.
>
> Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com>
> ---
> src/qemu/qemu_hotplug.c | 14 +++++++++++---
> src/util/virfile.h | 12 ++++++++++++
> 2 files changed, 23 insertions(+), 3 deletions(-)
>
> +++ b/src/util/virfile.h
> @@ -75,6 +75,18 @@ FILE *virFileFdopen(int *fdptr, const char
*mode) ATTRIBUTE_RETURN_CHECK;
> VIR_FILE_CLOSE_PRESERVE_ERRNO | \
> VIR_FILE_CLOSE_DONT_LOG))
>
> +static inline void vir_init_n_int(int *ptr, int count, int value)
> +{
> + int i;
> +
> + for (i = 0; i < count; i++)
> + ptr[i] = value;
> +}
Do we ever plan on using this for values other than '-1'?
It's probably too general for the fd purpose here only.
> +
> +/* Initialize an array of file descriptors to -1 */
> +# define VIR_INIT_N_FD(ptr, count) \
> + vir_init_n_int(ptr, count, -1)
Could also be spelled:
memset(ptr, -1, sizeof(*ptr) * count))
which goes back to why we need vir_init_n_int().
Also this would be possible.
I agree that the fix to qemu_hotplug.c to not close fd 0 on failure is
needed, but am not sure about the complexity of the virfile.h addition.
So a simple memset() would do? Having a macro for VIR_ALLOC_N_FD() would
probably be too much, eh?
Stefan
4:49 p.m.
On 03/10/2014 01:10 PM, Stefan Berger wrote:
Eric Blake <eblake(a)redhat.com> wrote on 03/10/2014 02:09:58
PM:
>> +/* Initialize an array of file descriptors to -1 */
>> +# define VIR_INIT_N_FD(ptr, count) \
>> + vir_init_n_int(ptr, count, -1)
>
> Could also be spelled:
>
> memset(ptr, -1, sizeof(*ptr) * count))
>
> which goes back to why we need vir_init_n_int().
Also this would be possible.
>
> I agree that the fix to qemu_hotplug.c to not close fd 0 on failure is
> needed, but am not sure about the complexity of the virfile.h addition.
>
So a simple memset() would do? Having a macro for VIR_ALLOC_N_FD() would
probably be too much, eh?
Indeed:
$ git grep memset.*1
src/conf/domain_conf.c: memset(max_idx, -1, sizeof(max_idx));
src/qemu/qemu_command.c: memset(tapfd, -1, tapfdSize *
sizeof(tapfd[0]));
src/qemu/qemu_command.c: memset(vhostfd, -1, vhostfdSize *
sizeof(vhostfd[0]));
So let's just copy that convention, instead of adding a new macro.
ACK to the qemu_hotplug fixes if you switch to raw memset.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:48 p.m.
Eric Blake <eblake(a)redhat.com> wrote on 03/10/2014 05:49:54 PM:
Indeed:
$ git grep memset.*1
src/conf/domain_conf.c: memset(max_idx, -1, sizeof(max_idx));
src/qemu/qemu_command.c: memset(tapfd, -1, tapfdSize *
sizeof(tapfd[0]));
src/qemu/qemu_command.c: memset(vhostfd, -1, vhostfdSize *
sizeof(vhostfd[0]));
So let's just copy that convention, instead of adding a new macro.
ACK to the qemu_hotplug fixes if you switch to raw memset.
Pushed.
4:23 a.m.
On 09.03.2014 00:29, Stefan Berger wrote:
From: Stefan Berger <stefanb(a)linux.vnet.ibm.com>
Avoid the freeing of an array of zero file descriptors in case
of error. Introduce a macro VIR_INIT_N_FD to initialize such
an array's elements to -1.
Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com>
---
Just a small nitpick, I'd reword the subject (something among
'qemu_hotplug: avoid freeing ..' or even
s/qemu_hotplug/qemuDomainAttachNetDevice/). And then I'd put link to the
bug on the first line of the commit message.
Michal
3908
days inactive
3911
days old
5 comments
3 participants
participants (3)
-
Eric Blake -
Michal Privoznik -
Stefan Berger