8:18 a.m.
Except not really. Only the first one is strictly related to that type
of <interface/>. The other one fixes a regression introduced in 8.4.0
and it just so happens that I'm able to reproduce it 100% on my
(SRIOV-less) machine.
Michal Prívozník (2):
virNetDevSaveNetConfig: Pass mode to virFileWriteStr()
qemuBuildInterfaceConnect: Initialize @tapfd array
src/qemu/qemu_command.c | 2 ++
src/util/virnetdev.c | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
--
2.35.1
8:18 a.m.
New subject: [PATCH 1/2] virNetDevSaveNetConfig: Pass mode to virFileWriteStr()
For some types of SRIOV interfaces we create a temporary file
where the state of the interface is saved before we start
modifying it. The file is used then to restore the original
configuration when the interface is no longer associated with any
guest. For writing the file virFileWriteStr() is used. However,
it's given wrong argument: the last argument is supposed to be
mode to create the file with but virNetDevSaveNetConfig() passes
open(2) flags (O_CREAT|O_TRUNC|O_WRONLY). We need the file to be
writable and readable by root only (0600). Therefore, pass that
mode instead of gibberish.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virnetdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 93f836cd92..6c6f6dee42 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1977,7 +1977,7 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
if (!(fileStr = virJSONValueToString(configJSON, true)))
return -1;
- if (virFileWriteStr(filePath, fileStr, O_CREAT|O_TRUNC|O_WRONLY) < 0) {
+ if (virFileWriteStr(filePath, fileStr, 0600) < 0) {
virReportSystemError(errno, _("Unable to preserve mac/vlan tag "
"for device = %s, vf = %d"), linkdev,
vf);
return -1;
--
2.35.1
9:01 a.m.
New subject: [PATCH 1/2] virNetDevSaveNetConfig: Pass mode to virFileWriteStr()
On Mon, Jun 13, 2022 at 15:18:13 +0200, Michal Privoznik wrote:
For some types of SRIOV interfaces we create a temporary file
where the state of the interface is saved before we start
modifying it. The file is used then to restore the original
configuration when the interface is no longer associated with any
guest. For writing the file virFileWriteStr() is used. However,
it's given wrong argument: the last argument is supposed to be
mode to create the file with but virNetDevSaveNetConfig() passes
open(2) flags (O_CREAT|O_TRUNC|O_WRONLY). We need the file to be
writable and readable by root only (0600). Therefore, pass that
mode instead of gibberish.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virnetdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
10:19 a.m.
New subject: [PATCH 1/2] virNetDevSaveNetConfig: Pass mode to virFileWriteStr()
On 6/13/22 9:18 AM, Michal Privoznik wrote:
For some types of SRIOV interfaces we create a temporary file
where the state of the interface is saved before we start
modifying it. The file is used then to restore the original
configuration when the interface is no longer associated with any
guest. For writing the file virFileWriteStr() is used. However,
it's given wrong argument: the last argument is supposed to be
mode to create the file with but virNetDevSaveNetConfig() passes
open(2) flags (O_CREAT|O_TRUNC|O_WRONLY). We need the file to be
writable and readable by root only (0600). Therefore, pass that
mode instead of gibberish.
Wow. This may be in competition for the longest living "how did this
ever work?" bug in the code :-/
Since my name was on the git blame for the most recent change to this
line, I had to figure out if it was really me that had
misunderstood/misused virFileWriteStr() so grievously (wouldn't be the
first or the last time). What I found was that this code had been moved
around by multiple different people (including me) since originally
being included in new code all the way back in commit cbd8227ee in June
2011.
Anyway, Peter has already acked it, but still
Reviewed-by: Laine Stump <laine(a)redhat.com>
8:18 a.m.
New subject: [PATCH 2/2] qemuBuildInterfaceConnect: Initialize @tapfd array
When creating a TAP interface we can end up with multiple FDs,
each representing one queue. However, these FDs must be
relabelled as they are then passed to QEMU. In case of
qemuBuildInterfaceConnect() we allocate the array for the FDs and
then let function corresponding to the <interface/> type to fill
the array with FDs. When any of the functions meets an error,
it's also responsible for closing previously opened FDs. However,
the functions take a shortcut: iterate through each member of the
array and close it (if it's non-negative). This assumes that the
array is initialized to negative values, which use to be the case
before rewrite in v8.4.0-rc1~170 but after it it's no longer the
case. Subsequently, "random" FDs are closed (okay, not that
random since the array is allocated via g_new0(), but hey - FD 0
is still valid FD and might be valuable, actually).
Fixes: 7a38d3946bc1a7ef0206f36dfe3dbf422fb8d578
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_command.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index be20053c0d..ecfe6020f3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8736,6 +8736,8 @@ qemuBuildInterfaceConnect(virDomainObj *vm,
size_t tapfdSize = net->driver.virtio.queues;
g_autofree int *tapfd = g_new0(int, tapfdSize + 1);
+ memset(tapfd, -1, (tapfdSize + 1) * sizeof(*tapfd));
+
if (tapfdSize == 0)
tapfdSize = 1;
--
2.35.1
8:30 a.m.
New subject: [PATCH 2/2] qemuBuildInterfaceConnect: Initialize @tapfd array
On 6/13/22 15:18, Michal Privoznik wrote:
When creating a TAP interface we can end up with multiple FDs,
each representing one queue. However, these FDs must be
relabelled as they are then passed to QEMU. In case of
qemuBuildInterfaceConnect() we allocate the array for the FDs and
then let function corresponding to the <interface/> type to fill
the array with FDs. When any of the functions meets an error,
it's also responsible for closing previously opened FDs. However,
the functions take a shortcut: iterate through each member of the
array and close it (if it's non-negative). This assumes that the
array is initialized to negative values, which use to be the case
before rewrite in v8.4.0-rc1~170 but after it it's no longer the
case. Subsequently, "random" FDs are closed (okay, not that
random since the array is allocated via g_new0(), but hey - FD 0
is still valid FD and might be valuable, actually).
Fixes: 7a38d3946bc1a7ef0206f36dfe3dbf422fb8d578
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
After some discussion in BZ I'm going to append:
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2075383#c18
Michal
9:04 a.m.
New subject: [PATCH 2/2] qemuBuildInterfaceConnect: Initialize @tapfd array
On Mon, Jun 13, 2022 at 15:18:14 +0200, Michal Privoznik wrote:
When creating a TAP interface we can end up with multiple FDs,
each representing one queue. However, these FDs must be
relabelled as they are then passed to QEMU. In case of
qemuBuildInterfaceConnect() we allocate the array for the FDs and
then let function corresponding to the <interface/> type to fill
the array with FDs. When any of the functions meets an error,
it's also responsible for closing previously opened FDs. However,
the functions take a shortcut: iterate through each member of the
array and close it (if it's non-negative). This assumes that the
array is initialized to negative values, which use to be the case
before rewrite in v8.4.0-rc1~170 but after it it's no longer the
case. Subsequently, "random" FDs are closed (okay, not that
random since the array is allocated via g_new0(), but hey - FD 0
is still valid FD and might be valuable, actually).
Fixes: 7a38d3946bc1a7ef0206f36dfe3dbf422fb8d578
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_command.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index be20053c0d..ecfe6020f3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8736,6 +8736,8 @@ qemuBuildInterfaceConnect(virDomainObj *vm,
size_t tapfdSize = net->driver.virtio.queues;
g_autofree int *tapfd = g_new0(int, tapfdSize + 1);
+ memset(tapfd, -1, (tapfdSize + 1) * sizeof(*tapfd));
+
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
but I'm adding to my todo list to fix the offending functions :)
9:42 a.m.
New subject: [PATCH 2/2] qemuBuildInterfaceConnect: Initialize @tapfd array
On 6/13/22 16:04, Peter Krempa wrote:
On Mon, Jun 13, 2022 at 15:18:14 +0200, Michal Privoznik wrote:
> When creating a TAP interface we can end up with multiple FDs,
> each representing one queue. However, these FDs must be
> relabelled as they are then passed to QEMU. In case of
> qemuBuildInterfaceConnect() we allocate the array for the FDs and
> then let function corresponding to the <interface/> type to fill
> the array with FDs. When any of the functions meets an error,
> it's also responsible for closing previously opened FDs. However,
> the functions take a shortcut: iterate through each member of the
> array and close it (if it's non-negative). This assumes that the
> array is initialized to negative values, which use to be the case
> before rewrite in v8.4.0-rc1~170 but after it it's no longer the
> case. Subsequently, "random" FDs are closed (okay, not that
> random since the array is allocated via g_new0(), but hey - FD 0
> is still valid FD and might be valuable, actually).
>
> Fixes: 7a38d3946bc1a7ef0206f36dfe3dbf422fb8d578
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_command.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index be20053c0d..ecfe6020f3 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -8736,6 +8736,8 @@ qemuBuildInterfaceConnect(virDomainObj *vm,
> size_t tapfdSize = net->driver.virtio.queues;
> g_autofree int *tapfd = g_new0(int, tapfdSize + 1);
>
> + memset(tapfd, -1, (tapfdSize + 1) * sizeof(*tapfd));
> +
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
but I'm adding to my todo list to fix the offending functions :)
Yeah, that might be worth fixing. Pushed thanks.
Michal
893
days inactive
893
days old
7 comments
4 participants
participants (4)
-
Laine Stump -
Michal Privoznik -
Michal Prívozník
-
Peter Krempa