8:45 a.m.
A bunch of functions opened the PCI device config as read-write
even though they only read from it.
Ján Tomko (5):
util: introduce virPCIDeviceConfigOpenInternal
util: Introduce virPCIDeviceConfigOpenWrite
util: introduce readonly attribute to virPCIDeviceConfigOpenInternal
util: introduce virPCIDeviceConfigOpenTry
util: default to read-only in virPCIDeviceConfigOpen
src/util/virpci.c | 40 +++++++++++++++++++++++++++++-----------
1 file changed, 29 insertions(+), 11 deletions(-)
--
2.20.1
8:45 a.m.
New subject: [libvirt] [PATCH 1/5] util: introduce virPCIDeviceConfigOpenInternal
A thin wrapper to allow creating new functions.
Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
---
src/util/virpci.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 61a6b359e5..f4be907a10 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -291,7 +291,7 @@ virPCIDeviceGetDriverPathAndName(virPCIDevicePtr dev, char **path,
char **name)
static int
-virPCIDeviceConfigOpen(virPCIDevicePtr dev, bool fatal)
+virPCIDeviceConfigOpenInternal(virPCIDevicePtr dev, bool fatal)
{
int fd;
@@ -314,6 +314,12 @@ virPCIDeviceConfigOpen(virPCIDevicePtr dev, bool fatal)
return fd;
}
+static int
+virPCIDeviceConfigOpen(virPCIDevicePtr dev, bool fatal)
+{
+ return virPCIDeviceConfigOpenInternal(dev, fatal);
+}
+
static void
virPCIDeviceConfigClose(virPCIDevicePtr dev, int cfgfd)
{
--
2.20.1
8:45 a.m.
New subject: [libvirt] [PATCH 2/5] util: Introduce virPCIDeviceConfigOpenWrite
Only a handful of function need write access to the PCI config
space. Create a wrapper function for those so that we can
open it read only by default.
Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
---
src/util/virpci.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index f4be907a10..c1dad1e69f 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -320,6 +320,12 @@ virPCIDeviceConfigOpen(virPCIDevicePtr dev, bool fatal)
return virPCIDeviceConfigOpenInternal(dev, fatal);
}
+static int
+virPCIDeviceConfigOpenWrite(virPCIDevicePtr dev)
+{
+ return virPCIDeviceConfigOpenInternal(dev, true);
+}
+
static void
virPCIDeviceConfigClose(virPCIDevicePtr dev, int cfgfd)
{
@@ -808,7 +814,7 @@ virPCIDeviceTrySecondaryBusReset(virPCIDevicePtr dev,
dev->name);
return -1;
}
- if ((parentfd = virPCIDeviceConfigOpen(parent, true)) < 0)
+ if ((parentfd = virPCIDeviceConfigOpenWrite(parent)) < 0)
goto out;
VIR_DEBUG("%s %s: doing a secondary bus reset", dev->id, dev->name);
@@ -957,7 +963,7 @@ virPCIDeviceReset(virPCIDevicePtr dev,
}
VIR_DEBUG("Resetting device %s", dev->name);
- if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0)
+ if ((fd = virPCIDeviceConfigOpenWrite(dev)) < 0)
goto cleanup;
if (virPCIDeviceInit(dev, fd) < 0)
--
2.20.1
8:45 a.m.
New subject: [libvirt] [PATCH 3/5] util: introduce readonly attribute to virPCIDeviceConfigOpenInternal
Allow wrappers to open PCI config as read-only.
Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
---
src/util/virpci.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index c1dad1e69f..e45dfbc631 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -291,11 +291,11 @@ virPCIDeviceGetDriverPathAndName(virPCIDevicePtr dev, char **path,
char **name)
static int
-virPCIDeviceConfigOpenInternal(virPCIDevicePtr dev, bool fatal)
+virPCIDeviceConfigOpenInternal(virPCIDevicePtr dev, bool readonly, bool fatal)
{
int fd;
- fd = open(dev->path, O_RDWR);
+ fd = open(dev->path, readonly ? O_RDONLY : O_RDWR);
if (fd < 0) {
if (fatal) {
@@ -317,13 +317,13 @@ virPCIDeviceConfigOpenInternal(virPCIDevicePtr dev, bool fatal)
static int
virPCIDeviceConfigOpen(virPCIDevicePtr dev, bool fatal)
{
- return virPCIDeviceConfigOpenInternal(dev, fatal);
+ return virPCIDeviceConfigOpenInternal(dev, false, fatal);
}
static int
virPCIDeviceConfigOpenWrite(virPCIDevicePtr dev)
{
- return virPCIDeviceConfigOpenInternal(dev, true);
+ return virPCIDeviceConfigOpenInternal(dev, false, true);
}
static void
--
2.20.1
8:45 a.m.
New subject: [libvirt] [PATCH 4/5] util: introduce virPCIDeviceConfigOpenTry
For callers that only need read-only access and don't want
an error reported.
Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
---
src/util/virpci.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index e45dfbc631..2758ee6f49 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -320,6 +320,12 @@ virPCIDeviceConfigOpen(virPCIDevicePtr dev, bool fatal)
return virPCIDeviceConfigOpenInternal(dev, false, fatal);
}
+static int
+virPCIDeviceConfigOpenTry(virPCIDevicePtr dev)
+{
+ return virPCIDeviceConfigOpenInternal(dev, true, false);
+}
+
static int
virPCIDeviceConfigOpenWrite(virPCIDevicePtr dev)
{
@@ -692,7 +698,7 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void
*data)
if (dev->address.domain != check->address.domain)
return 0;
- if ((fd = virPCIDeviceConfigOpen(check, false)) < 0)
+ if ((fd = virPCIDeviceConfigOpenTry(check)) < 0)
return 0;
/* Is it a bridge? */
@@ -740,7 +746,7 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void
*data)
int bestfd;
uint8_t best_secondary;
- if ((bestfd = virPCIDeviceConfigOpen(*best, false)) < 0)
+ if ((bestfd = virPCIDeviceConfigOpenTry(*best)) < 0)
goto cleanup;
best_secondary = virPCIDeviceRead8(*best, bestfd, PCI_SECONDARY_BUS);
virPCIDeviceConfigClose(*best, bestfd);
--
2.20.1
8:45 a.m.
New subject: [libvirt] [PATCH 5/5] util: default to read-only in virPCIDeviceConfigOpen
All the callers left require virPCIDeviceConfigOpen to be fatal
and only use read-only access to the config file.
Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
---
src/util/virpci.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 2758ee6f49..7ca755c1a8 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -315,9 +315,9 @@ virPCIDeviceConfigOpenInternal(virPCIDevicePtr dev, bool readonly,
bool fatal)
}
static int
-virPCIDeviceConfigOpen(virPCIDevicePtr dev, bool fatal)
+virPCIDeviceConfigOpen(virPCIDevicePtr dev)
{
- return virPCIDeviceConfigOpenInternal(dev, false, fatal);
+ return virPCIDeviceConfigOpenInternal(dev, true, true);
}
static int
@@ -2426,7 +2426,7 @@ virPCIDeviceDownstreamLacksACS(virPCIDevicePtr dev)
int ret = 0;
uint16_t device_class;
- if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0)
+ if ((fd = virPCIDeviceConfigOpen(dev)) < 0)
return -1;
if (virPCIDeviceInit(dev, fd) < 0) {
@@ -3168,7 +3168,7 @@ virPCIDeviceIsPCIExpress(virPCIDevicePtr dev)
int fd;
int ret = -1;
- if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0)
+ if ((fd = virPCIDeviceConfigOpen(dev)) < 0)
return ret;
if (virPCIDeviceInit(dev, fd) < 0)
@@ -3188,7 +3188,7 @@ virPCIDeviceHasPCIExpressLink(virPCIDevicePtr dev)
int ret = -1;
uint16_t cap, type;
- if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0)
+ if ((fd = virPCIDeviceConfigOpen(dev)) < 0)
return ret;
if (virPCIDeviceInit(dev, fd) < 0)
@@ -3216,7 +3216,7 @@ virPCIDeviceGetLinkCapSta(virPCIDevicePtr dev,
int fd;
int ret = -1;
- if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0)
+ if ((fd = virPCIDeviceConfigOpen(dev)) < 0)
return ret;
if (virPCIDeviceInit(dev, fd) < 0)
@@ -3254,7 +3254,7 @@ int virPCIGetHeaderType(virPCIDevicePtr dev, int *hdrType)
*hdrType = -1;
- if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0)
+ if ((fd = virPCIDeviceConfigOpen(dev)) < 0)
return -1;
type = virPCIDeviceRead8(dev, fd, PCI_HEADER_TYPE);
--
2.20.1
3:10 a.m.
New subject: [libvirt] [PATCH 5/5] util: default to read-only in virPCIDeviceConfigOpen
On Tue, Aug 13, 2019 at 03:45:19PM +0200, Ján Tomko wrote:
All the callers left require virPCIDeviceConfigOpen to be fatal
and only use read-only access to the config file.
Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
---
FYI this patch broke make check, both virpcitest and virhostdevtest.
Erik
4:09 a.m.
New subject: [libvirt] [PATCH 5/5] util: default to read-only in virPCIDeviceConfigOpen
On Thu, Aug 15, 2019 at 10:10:06 +0200, Erik Skultety wrote:
On Tue, Aug 13, 2019 at 03:45:19PM +0200, Ján Tomko wrote:
> All the callers left require virPCIDeviceConfigOpen to be fatal
> and only use read-only access to the config file.
>
> Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
> ---
FYI this patch broke make check, both virpcitest and virhostdevtest.
Actually it's the previous patch according to my bisection and it
happens only with gcc. Clang works fine. This hints to some inlining
weridness happening together with the pci mocking code.
7:20 a.m.
On 8/13/19 3:45 PM, Ján Tomko wrote:
A bunch of functions opened the PCI device config as read-write
even though they only read from it.
Ján Tomko (5):
util: introduce virPCIDeviceConfigOpenInternal
util: Introduce virPCIDeviceConfigOpenWrite
util: introduce readonly attribute to virPCIDeviceConfigOpenInternal
util: introduce virPCIDeviceConfigOpenTry
util: default to read-only in virPCIDeviceConfigOpen
src/util/virpci.c | 40 +++++++++++++++++++++++++++++-----------
1 file changed, 29 insertions(+), 11 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
1959
days inactive
1961
days old
8 comments
4 participants
participants (4)
-
Erik Skultety -
Ján Tomko -
Michal Privoznik -
Peter Krempa