[libvirt] [PATCH 5/5] nodedev: Protect every access to udev_monitor by locking the driver
Since @udev_monitor isn't immutable within the driver, we need to
protect every access to it by locking the driver, so that no spurious
action changing the pointer while one thread is actively using it might
occur. This patch just takes some precaution measures.
Signed-off-by: Erik Skultety
---
src/node_device/node_device_udev.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 7ecb330df..0643a5845 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1623,9 +1623,13 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
void *data ATTRIBUTE_UNUSED)
{
struct udev_device *device = NULL;
-struct udev_monitor *udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
+struct udev_monitor *udev_monitor = NULL;
int udev_fd = -1;
+nodeDeviceLock();
+if (!(udev_monitor = DRV_STATE_UDEV_MONITOR(driver)))
+goto error;
+
udev_fd = udev_monitor_get_fd(udev_monitor);
if (fd != udev_fd) {
udevPrivate *priv = driver->privateData;
@@ -1640,22 +1644,26 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
* the same error multiple times
*/
virEventRemoveHandle(priv->watch);
-
-goto cleanup;
+goto error;
}
device = udev_monitor_receive_device(udev_monitor);
if (device == NULL) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("udev_monitor_receive_device returned NULL"));
-goto cleanup;
+goto error;
}
+nodeDeviceUnlock();
udevHandleOneDevice(device);
cleanup:
udev_device_unref(device);
return;
+
+ error:
+nodeDeviceUnlock();
+goto cleanup;
}
--
2.13.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/5] nodedev: Clear the udev_monitor reference once unref'd
Since we only have one udev_monitor reference throughout libvirt, we
should either clear the pointer we copy around to prevent invalid memory
access once we unref this single reference, or start using reference
counting provided by libudev. This patch follows the former and only
clears the pointer.
Signed-off-by: Erik Skultety
---
src/node_device/node_device_udev.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index ea10dc3ce..cd19e79c1 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1581,6 +1581,7 @@ nodeStateCleanup(void)
if (udev_monitor != NULL) {
udev = udev_monitor_get_udev(udev_monitor);
udev_monitor_unref(udev_monitor);
+priv->udev_monitor = NULL;
}
}
--
2.13.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/5] nodedev: mdev: Report an error when virFileResolveLink fails
It might happen that virFileResolveLinkHelper fails on the lstat system
call. virFileResolveLink expects the caller to report an error when it
fails, however this wasn't the case for udevProcessMediatedDevice.
Signed-off-by: Erik Skultety
---
src/node_device/node_device_udev.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 4762f1969..80c346e96 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1121,8 +1121,10 @@ udevProcessMediatedDevice(struct udev_device *dev,
if (virAsprintf(&linkpath, "%s/mdev_type", udev_device_get_syspath(dev)) <
0)
goto cleanup;
-if (virFileResolveLink(linkpath, &canonicalpath) < 0)
+if (virFileResolveLink(linkpath, &canonicalpath) < 0) {
+virReportSystemError(errno, _("failed to resolve '%s'"), linkpath);
goto cleanup;
+}
if (VIR_STRDUP(data->type, last_component(canonicalpath)) < 0)
goto cleanup;
--
2.13.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/5] nodedev: Introduce udevHandleOneDevice
Let this new method handle the device object we obtained from the
monitor in order to enhance readability.
Signed-off-by: Erik Skultety
---
src/node_device/node_device_udev.c | 31 ++-
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index cd19e79c1..7ecb330df 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1599,6 +1599,23 @@ nodeStateCleanup(void)
}
+static int
+udevHandleOneDevice(struct udev_device *device)
+{
+const char *action = udev_device_get_action(device);
+
+VIR_DEBUG("udev action: '%s'", action);
+
+if (STREQ(action, "add") || STREQ(action, "change"))
+return udevAddOneDevice(device);
+
+if (STREQ(action, "remove"))
+return udevRemoveOneDevice(device);
+
+return 0;
+}
+
+
static void
udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
int fd,
@@ -1607,7 +1624,6 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
{
struct udev_device *device = NULL;
struct udev_monitor *udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
-const char *action = NULL;
int udev_fd = -1;
udev_fd = udev_monitor_get_fd(udev_monitor);
@@ -1635,18 +1651,7 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
goto cleanup;
}
-action = udev_device_get_action(device);
-VIR_DEBUG("udev action: '%s'", action);
-
-if (STREQ(action, "add") || STREQ(action, "change")) {
-udevAddOneDevice(device);
-goto cleanup;
-}
-
-if (STREQ(action, "remove")) {
-udevRemoveOneDevice(device);
-goto cleanup;
-}
+udevHandleOneDevice(device);
cleanup:
udev_device_unref(device);
--
2.13.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/5] nodedev: udev: Remove the udevEventHandleCallback on fatal error
So we have a sanity check for the udev monitor fd. Theoretically, it
could happen that the udev monitor fd changes (due to our own wrongdoing,
hence the 'sanity' here) and if that happens it means we are handling an
event from a different entity than we think, thus we should remove the
handle if someone somewhere somehow hits this hypothetical case.
Signed-off-by: Erik Skultety
---
src/node_device/node_device_udev.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 80c346e96..ea10dc3ce 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1611,10 +1611,19 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
udev_fd = udev_monitor_get_fd(udev_monitor);
if (fd != udev_fd) {
+udevPrivate *priv = driver->privateData;
+
virReportError(VIR_ERR_INTERNAL_ERROR,
_("File descriptor returned by udev %d does not "
"match node device file descriptor %d"),
fd, udev_fd);
+
+/* this is a non-recoverable error, let's remove the handle, so that we
+ * don't get in here again because of some spurious behaviour and
report
+ * the same error multiple times
+ */
+virEventRemoveHandle(priv->watch);
+
goto cleanup;
}
--
2.13.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/5] A few fixes and improvements to the nodedev driver
These are just the few I came across when working on another nodedev-related series. Erik Skultety (5): nodedev: mdev: Report an error when virFileResolveLink fails nodedev: udev: Remove the udevEventHandleCallback on fatal error nodedev: Clear the udev_monitor reference once unref'd nodedev: Introduce udevHandleOneDevice nodedev: Protect every access to udev_monitor by locking the driver src/node_device/node_device_udev.c | 59 +++--- 1 file changed, 42 insertions(+), 17 deletions(-) -- 2.13.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: we prefer C89 comment styles over C99
On Tue, Jul 25, 2017 at 11:14:10PM +0200, Pavel Hrdina wrote:
Introduced by commit 'a7bc2c8cfd6f'.
Signed-off-by: Pavel Hrdina
---
Pushed under trivial rule.
src/qemu/qemu_domain.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index dcdbfc9701..5c073027db 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7264,7 +7264,8 @@ qemuDomainPrepareChannel(virDomainChrDefPtr channel,
"%s/%s", domainChannelTargetDir,
channel->target.name) < 0)
return -1;
-} else {// Generate a unique name
+} else {
+/* Generate a unique name */
Oh, thanks for catching this. You could also remove the comment
altogether since the code is pretty self-explanatory, I guess.
Unless you already pushed it, s/we // in $SUBJ.
if (virAsprintf(&channel->source->data.nix.path,
"%s/vioser-%02d-%02d-%02d.sock",
domainChannelTargetDir,
--
2.13.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: we prefer C89 comment styles over C99
Introduced by commit 'a7bc2c8cfd6f'.
Signed-off-by: Pavel Hrdina
---
Pushed under trivial rule.
src/qemu/qemu_domain.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index dcdbfc9701..5c073027db 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7264,7 +7264,8 @@ qemuDomainPrepareChannel(virDomainChrDefPtr channel,
"%s/%s", domainChannelTargetDir,
channel->target.name) < 0)
return -1;
-} else {// Generate a unique name
+} else {
+/* Generate a unique name */
if (virAsprintf(&channel->source->data.nix.path,
"%s/vioser-%02d-%02d-%02d.sock",
domainChannelTargetDir,
--
2.13.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] Generate unique socket file
On Tue, Jul 25, 2017 at 09:33:50AM -0500, Scott Garfinkle wrote:
> It's possible to have more than one unnamed virtio-serial unix channel.
> We need to generate a unique name for each channel. Currently, we use
> ".../unknown.sock" for all of them. Better practice would be to specify
> an explicit target path name; however, in the absence of that, we need
> uniqueness in the names we generate internally.
>
> Before the changes we'd get /var/lib/libvirt/qemu/channel/target/unknown.sock
> for each instance of
>
>
>
>
>
> Now, we get vioser-00-00-01.sock, vioser-00-00-02.sock, etc.
>
> Changes from v1: new socket name
>
> Signed-off-by: Scott Garfinkle
>
> ---
> src/qemu/qemu_domain.c | 24
> +++---
> .../qemuxml2argv-channel-virtio-unix.args | 2 +-
> 2 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 78e75f1..7a9958d 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7178,18 +7178,28 @@ int
> qemuDomainPrepareChannel(virDomainChrDefPtr channel,
> const char *domainChannelTargetDir)
> {
> -if (channel->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO &&
> -channel->source->type == VIR_DOMAIN_CHR_TYPE_UNIX &&
> -!channel->source->data.nix.path) {
> +if (channel->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO ||
> +channel->source->type != VIR_DOMAIN_CHR_TYPE_UNIX ||
> +channel->source->data.nix.path)
> +return 0;
> +
> +if (channel->target.name) {
> if (virAsprintf(&channel->source->data.nix.path,
> "%s/%s", domainChannelTargetDir,
> -channel->target.name ? channel->target.name
> -: "unknown.sock") < 0)
> +channel->target.name) < 0)
> +return -1;
> +} else {// Generate a unique name
We don't allow this type of comment, since this patch is already pushed,
I'll fix it.
Pavel
> +if (virAsprintf(&channel->source->data.nix.path,
> +"%s/vioser-%02d-%02d-%02d.sock",
> +domainChannelTargetDir,
> +channel->info.addr.vioserial.controller,
> +channel->info.addr.vioserial.bus,
> +channel->info.addr.vioserial.port) < 0)
> return -1;
> -
> -channel->source->data.nix.listen = true;
> }
>
> +channel->source->data.nix.listen = true;
> +
> return 0;
> }
>
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.args
> b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.args
> index 2b72965..8e0452a 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.args
> @@ -29,7 +29,7 @@
> path=/tmp/channel/domain--1-QEMUGuest1/org.qemu.guest_agent.0,server,nowait \
> -device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,\
> id=channel0,name=org.qemu.guest_agent.0 \
> -chardev socket,id=charchannel1,\
> -path=/tmp/channel/domain--1-QEMUGuest1/unknown.sock,server,nowait \
> +path=/tmp/channel/domain--1-QEMUGuest1/vioser-00-00-02.sock,server,nowait \
> -device virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel1,\
> id=channel1 \
> -chardev socket,id=charchannel2,path=/tmp/channel/domain--1-QEMUGuest1/ble,\
> --
> 1.8.3.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] Generate unique socket file
On Tue, Jul 25, 2017 at 09:33:50AM -0500, Scott Garfinkle wrote:
It's possible to have more than one unnamed virtio-serial unix channel.
We need to generate a unique name for each channel. Currently, we use
".../unknown.sock" for all of them. Better practice would be to specify
an explicit target path name; however, in the absence of that, we need
uniqueness in the names we generate internally.
Before the changes we'd get /var/lib/libvirt/qemu/channel/target/unknown.sock
for each instance of
Now, we get vioser-00-00-01.sock, vioser-00-00-02.sock, etc.
Changes from v1: new socket name
Signed-off-by: Scott Garfinkle
---
src/qemu/qemu_domain.c | 24 +++---
.../qemuxml2argv-channel-virtio-unix.args | 2 +-
2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 78e75f1..7a9958d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7178,18 +7178,28 @@ int
qemuDomainPrepareChannel(virDomainChrDefPtr channel,
const char *domainChannelTargetDir)
{
-if (channel->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO &&
-channel->source->type == VIR_DOMAIN_CHR_TYPE_UNIX &&
-!channel->source->data.nix.path) {
+if (channel->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO ||
+channel->source->type != VIR_DOMAIN_CHR_TYPE_UNIX ||
+channel->source->data.nix.path)
+return 0;
+
Apart from the indentation right here, it looks fine. I'll fix this up
and push it in a while.
Reviewed-by: Martin Kletzander
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] Generate unique socket file
It's possible to have more than one unnamed virtio-serial unix channel.
We need to generate a unique name for each channel. Currently, we use
".../unknown.sock" for all of them. Better practice would be to specify
an explicit target path name; however, in the absence of that, we need
uniqueness in the names we generate internally.
Before the changes we'd get /var/lib/libvirt/qemu/channel/target/unknown.sock
for each instance of
Now, we get vioser-00-00-01.sock, vioser-00-00-02.sock, etc.
Changes from v1: new socket name
Signed-off-by: Scott Garfinkle
---
src/qemu/qemu_domain.c | 24 +++---
.../qemuxml2argv-channel-virtio-unix.args | 2 +-
2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 78e75f1..7a9958d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7178,18 +7178,28 @@ int
qemuDomainPrepareChannel(virDomainChrDefPtr channel,
const char *domainChannelTargetDir)
{
-if (channel->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO &&
-channel->source->type == VIR_DOMAIN_CHR_TYPE_UNIX &&
-!channel->source->data.nix.path) {
+if (channel->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO ||
+channel->source->type != VIR_DOMAIN_CHR_TYPE_UNIX ||
+channel->source->data.nix.path)
+return 0;
+
+if (channel->target.name) {
if (virAsprintf(&channel->source->data.nix.path,
"%s/%s", domainChannelTargetDir,
-channel->target.name ? channel->target.name
-: "unknown.sock") < 0)
+channel->target.name) < 0)
+return -1;
+} else {// Generate a unique name
+if (virAsprintf(&channel->source->data.nix.path,
+"%s/vioser-%02d-%02d-%02d.sock",
+domainChannelTargetDir,
+channel->info.addr.vioserial.controller,
+channel->info.addr.vioserial.bus,
+channel->info.addr.vioserial.port) < 0)
return -1;
-
-channel->source->data.nix.listen = true;
}
+channel->source->data.nix.listen = true;
+
return 0;
}
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.args
b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.args
index 2b72965..8e0452a 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.args
@@ -29,7 +29,7 @@
path=/tmp/channel/domain--1-QEMUGuest1/org.qemu.guest_agent.0,server,nowait \
-device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,\
id=channel0,name=org.qemu.guest_agent.0 \
-chardev socket,id=charchannel1,\
-path=/tmp/channel/domain--1-QEMUGuest1/unknown.sock,server,nowait \
+path=/tmp/channel/domain--1-QEMUGuest1/vioser-00-00-02.sock,server,nowait \
-device virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel1,\
id=channel1 \
-chardev socket,id=charchannel2,path=/tmp/channel/domain--1-QEMUGuest1/ble,\
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH glib] Don't set LC_ALL=C during build as that breaks python apps
Hello, Am 25.07.2017 um 14:07 schrieb Daniel P. Berrange: > Setting LC_ALL=C breaks python apps doing I/O on UTF-8 source > files. In particular this broke glib-mkenums > > GEN libvirt-gconfig-enum-types.h > Traceback (most recent call last): > File "/usr/bin/glib-mkenums", line 669, in > process_file(fname) > File "/usr/bin/glib-mkenums", line 406, in process_file > line = curfile.readline() > File "/usr/lib64/python3.6/encodings/ascii.py", line 26, in decode > return codecs.ascii_decode(input, self.errors)[0] > UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 849: > ordinal not in range(128) What about using "C.UTF-8" instead, which us the same as "C" but with "UTF-8" encoding? Maybe ancient RedHat still doesn't know about it ... Philipp -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable
>> With your suggestion, I'd have to change all virObjectLock() in >> src/conf/virnetworkobj.c to virObjectLockWrite() (all that lock the hash >> table, not network object itself). >> > > Obviously I hadn't completely through everything... > > But perhaps this also proves that under the covers we could have just > converted virObjectLockable to be a virObjectRWLockable without creating > the new class. Especially since the former patch 1 has been reverted and > there's no difference between virObjectLockableNew and > virObjectRWLockableNew other than which class was used to initialize. > > If they were combined and just the new API to perform the RW lock was > added, then for paths that want to use read locks: > >if (!virObjectLockRead(obj)) > error and fail > >... > > IOW: At this point in time - what is the purpose of a separate > virObjectRWLockableClass? > > nm: I needed to keep excavating... Still I think not using overloaded Lock/Unlock in order to allow new functions to return a failure status would be better. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 2/2] qemu: Default hwclock source for sPAPR to RTC
QEMU fails to launch a sPAPR guest with clock sources other that RTC.
Internally qemu only uses RTC timer for hwclock. This patch reports
the right error message instead of qemu erroring out when any other
timer other than RTC is used.
Signed-off-by: Kothapally Madhu Pavan
---
src/qemu/qemu_domain.c | 8
1 file changed, 8 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e5e4208..b3cfb6c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3103,6 +3103,14 @@ qemuDomainDefValidate(const virDomainDef *def,
virArchToString(def->os.arch));
goto cleanup;
}
+/* /* Only RTC timer is supported as hwclock for sPAPR machines */
+if (ARCH_IS_PPC64(def->os.arch) && timer->name !=
VIR_DOMAIN_TIMER_NAME_RTC) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported clock timer '%s' for %s
architecture"),
+
virDomainTimerNameTypeToString(def->clock.timers[i]->name),
+ virArchToString(def->os.arch));
+goto cleanup;
+}
}
if (def->mem.min_guarantee) {
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu_cgroup: Remove unnecessary virQEMUDriverPtr arguments
Since commit 2e6ecba1bcac, the pointer to the qemu driver is saved in
domain object's private data and hence does not have to be passed as
yet another parameter if domain object is already one of them.
This is a first (example) patch of this kind of clean up, others will
hopefully follow.
Signed-off-by: Martin Kletzander
---
src/qemu/qemu_cgroup.c | 33 ++---
src/qemu/qemu_cgroup.h | 6 ++
src/qemu/qemu_process.c | 4 ++--
3 files changed, 18 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 7355527c1cc5..6fc413098ae8 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -604,8 +604,7 @@ qemuTeardownChardevCgroup(virDomainObjPtr vm,
static int
-qemuSetupDevicesCgroup(virQEMUDriverPtr driver,
- virDomainObjPtr vm)
+qemuSetupDevicesCgroup(virDomainObjPtr vm)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
virQEMUDriverConfigPtr cfg = NULL;
@@ -644,7 +643,7 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver,
if (rv < 0)
goto cleanup;
-cfg = virQEMUDriverGetConfig(driver);
+cfg = virQEMUDriverGetConfig(priv->driver);
deviceACL = cfg->cgroupDeviceACL ?
(const char *const *)cfg->cgroupDeviceACL :
defaultDeviceACL;
@@ -769,8 +768,7 @@ qemuSetupCpusetCgroup(virDomainObjPtr vm)
static int
-qemuSetupCpuCgroup(virQEMUDriverPtr driver,
- virDomainObjPtr vm)
+qemuSetupCpuCgroup(virDomainObjPtr vm)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
virObjectEventPtr event = NULL;
@@ -806,7 +804,7 @@ qemuSetupCpuCgroup(virQEMUDriverPtr driver,
event = virDomainEventTunableNewFromObj(vm, eventParams,
eventNparams);
}
-qemuDomainEventQueue(driver, event);
+qemuDomainEventQueue(priv->driver, event);
}
return 0;
@@ -814,16 +812,15 @@ qemuSetupCpuCgroup(virQEMUDriverPtr driver,
static int
-qemuInitCgroup(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuInitCgroup(virDomainObjPtr vm,
size_t nnicindexes,
int *nicindexes)
{
int ret = -1;
qemuDomainObjPrivatePtr priv = vm->privateData;
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(priv->driver);
-if (!virQEMUDriverIsPrivileged(driver))
+if (!virQEMUDriverIsPrivileged(priv->driver))
goto done;
if (!virCgroupAvailable())
@@ -954,14 +951,13 @@ qemuRestoreCgroupState(virDomainObjPtr vm)
}
int
-qemuConnectCgroup(virQEMUDriverPtr driver,
- virDomainObjPtr vm)
+qemuConnectCgroup(virDomainObjPtr vm)
{
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
qemuDomainObjPrivatePtr priv = vm->privateData;
+virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(priv->driver);
int ret = -1;
-if (!virQEMUDriverIsPrivileged(driver))
+if (!virQEMUDriverIsPrivileged(priv->driver))
goto done;
if (!virCgroupAvailable())
@@ -991,8 +987,7 @@ qemuConnectCgroup(virQEMUDriverPtr driver,
}
int
-qemuSetupCgroup(virQEMUDriverPtr driver,
-virDomainObjPtr vm,
+qemuSetupCgroup(virDomainObjPtr vm,
size_t nnicindexes,
int *nicindexes)
{
@@ -1005,13 +1000,13 @@ qemuSetupCgroup(virQEMUDriverPtr driver,
return -1;
}
-if (qemuInitCgroup(driver, vm, nnicindexes, nicindexes) < 0)
+if (qemuInitCgroup(vm, nnicindexes, nicindexes) < 0)
return -1;
if (!priv->cgroup)
return 0;
-if (qemuSetupDevicesCgroup(driver, vm) < 0)
+if (qemuSetupDevicesCgroup(vm) < 0)
goto cleanup;
if (qemuSetupBlkioCgroup(vm) < 0)
@@ -1020,7 +1015,7 @@ qemuSetupCgroup(virQEMUDriverPtr driver,
if (qemuSetupMemoryCgroup(vm) < 0)
goto cleanup;
-if (qemuSetupCpuCgroup(driver, vm) < 0)
+if (qemuSetupCpuCgroup(vm) < 0)
goto cleanup;
if (qemuSetupCpusetCgroup(vm) < 0)
diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h
index d016ce29d87e..3fc1583612e1 100644
--- a/src/qemu/qemu_cgroup.h
+++ b/src/qemu/qemu_cgroup.h
@@ -55,10 +55,8 @@ int qemuSetupChardevCgroup(virDomainObjPtr vm,
virDomainChrDefPtr dev);
int qemuTeardownChardevCgroup(virDomainObjPtr vm,
virDomainChrDefPtr dev);
-int qemuConnectCgroup(virQEMUDriverPtr driver,
- virDomainObjPtr vm);
-int qemuSetupCgroup(virQEMUDriverPtr driver,
-virDomainObjPtr vm,
+int qemuConnectCgroup(virDomainObjPtr vm);
+int qemuSetupCgroup(virDomainObjPtr vm,
size_t nnicindexes,
int *nicindexes);
int qemuSetupCpusetMems(virDomainObjPtr vm);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 87ab85fdb476..535c16b03705 100644
--- a/src/qemu/qemu_process.c
+++ b/src/q
[libvirt] Plans for next release
The end of the month is next week, so if we want to release around Aug 1st, I think we should enter freeze on Thursday morning, then I could make rc2 on Saturday and we could have the release on the Tuesday or Wed. Usually this is not the busiest time of the year but if this is a problem please raise this, thanks, Daniel -- Daniel Veillard | Red Hat Developers Tools http://developer.redhat.com/ veill...@redhat.com | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable
On 07/25/2017 10:25 AM, Michal Privoznik wrote:
> On 07/25/2017 02:13 PM, John Ferlan wrote:
>>
>>
>> On 07/25/2017 03:45 AM, Michal Privoznik wrote:
>>> On 07/24/2017 05:22 PM, John Ferlan wrote:
[...]
> /**
> * virObjectLock:
> - * @anyobj: any instance of virObjectLockablePtr
> + * @anyobj: any instance of virObjectLockable or virObjectRWLockable
> *
> - * Acquire a lock on @anyobj. The lock must be
> - * released by virObjectUnlock.
> + * Acquire a lock on @anyobj. The lock must be released by
> + * virObjectUnlock. In case the passed object is instance of
> + * virObjectRWLockable a write lock is acquired.
> *
> * The caller is expected to have acquired a reference
> * on the object before locking it (eg virObjectRef).
> @@ -340,31 +382,69 @@ virObjectGetLockableObj(void *anyobj)
> void
> virObjectLock(void *anyobj)
> {
> -virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
> +if (virObjectIsClass(anyobj, virObjectLockableClass)) {
> +virObjectLockablePtr obj = anyobj;
> +virMutexLock(&obj->lock);
> +} else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
> +virObjectRWLockablePtr obj = anyobj;
> +virRWLockWrite(&obj->lock);
> +} else {
> +virObjectPtr obj = anyobj;
> +VIR_WARN("Object %p (%s) is not a virObjectLockable "
> + "nor virObjectRWLockable instance",
> + anyobj, obj ? obj->klass->name : "(unknown)");
> +}
> +}
>
> -if (!obj)
> -return;
>
> -virMutexLock(&obj->lock);
> +/**
> + * virObjectLockRead:
> + * @anyobj: any instance of virObjectRWLockablePtr
> + *
> + * Acquire a read lock on @anyobj. The lock must be
> + * released by virObjectUnlock.
> + *
> + * The caller is expected to have acquired a reference
> + * on the object before locking it (eg virObjectRef).
> + * The object must be unlocked before releasing this
> + * reference.
> + */
> +void
> +virObjectLockRead(void *anyobj)
> +{
> +if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
> +virObjectRWLockablePtr obj = anyobj;
> +virRWLockRead(&obj->lock);
> +} else {
> +virObjectPtr obj = anyobj;
> +VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
> + anyobj, obj ? obj->klass->name : "(unknown)");
> +}
> }
>
Hopefully no one "confuses" which object is which or no one starts using
virObjectLockRead for a virObjectLockable and expects that read lock to
be in place (or error out) or gasp actually wait for a write lock to
release the lock as this does not error out.
>>>
>>> This could have already happened if one would pass bare virObject to
>>> virObjectLock().
>>>
>>
>> Consider passing a non virObject (such as what happened during an early
>> change to the code for me - a virHashTablePtr) to virObjectLock...
>
> Well, that's what happens in C to functions accepting void pointer. In
> this sense yes, we are not true OOP - compiler would catch that you're
> calling a method that is not defined for the class. But since we're
> implementing OOP in runtime, we are able to catch OOP related problem
> only at runtime. I'm not sure it is something we can check for at
> compile time. And I think other C libraries that re-implement OOP (like
> glib) do just the same as us.
>
Agreed - I cannot think of a way to capture this at compile time, but as
part of that virObject series I did add some code to reduce the chance
of some bad things happening during run time. They don't completely
eliminate the possibility that someone calls virObject{Lock|Unlock}
using an @obj that isn't a virObjectLockable. But it does protect
against someone using virObject{Ref|Unref} in order to avoid an
virAtomicIntInc or a virAtomicIntDecAndTest on a random @obj field (plus
some worse settings on random fields if for some reason @lastRef is true).
I hadn't come up with a good way to handle virObject{Lock|Unlock}, but
this series jiggled the memory a bit and got me thinking...
>>
>> In any case, we'll have to be more vigilant now to know that only
>> ObjList @obj's for virdomainobjlist can use this new API.
>
> Why? Maybe I'm misunderstanding what you're saying, but I think that
> network driver, and basically any other driver which uses vir*ObjList
> can use it. And not just lists. Other objects too - e.g. qemuCaps,
> virDomainCaps, etc (I really roughly skimmed through users of
> virObjectLockable).
Ok - wasn't as clear as I should have been... True, as long as the
_vir*ObjList uses virObjectRWLockable and the vir*ObjListNew() uses
virObjectRWLockableNew. Similar for other objects that want to do the
right thing.
I was considering someone that sees virOb
[libvirt] [PATCH v3 1/2] qemu: Restrict usage of hpet and kvm.pit timers by unsupported architectures
hpet and kvm.pit clock timers are specific to x86 architecture and
are not suppose to be used in other architectures. This patch restricts
the usage of hpet and kvm.pit timers in unsupported architectures.
Signed-off-by: Kothapally Madhu Pavan
---
src/qemu/qemu_domain.c | 16
1 file changed, 16 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2c8c9a7..e5e4208 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3083,12 +3083,28 @@ qemuDomainDefValidate(const virDomainDef *def,
virQEMUCapsPtr qemuCaps = NULL;
unsigned int topologycpus;
int ret = -1;
+size_t i;
if (!(qemuCaps = virQEMUCapsCacheLookup(caps,
driver->qemuCapsCache,
def->emulator)))
goto cleanup;
+/* Restrict usage of unsupported clock sources */
+for (i = 0; i < def->clock.ntimers; i++) {
+virDomainTimerDefPtr timer = def->clock.timers[i];
+if ((!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_HPET)) &&
+ (timer->name == VIR_DOMAIN_TIMER_NAME_HPET)) ||
+(!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_KVM_PIT)) &&
+ (timer->name == VIR_DOMAIN_TIMER_NAME_PIT))) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported clock timer '%s' for %s
architecture"),
+
virDomainTimerNameTypeToString(def->clock.timers[i]->name),
+ virArchToString(def->os.arch));
+goto cleanup;
+}
+}
+
if (def->mem.min_guarantee) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Parameter 'min_guarantee' not supported by QEMU."));
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] storage: Disallow usage of the HBA for a fc_host backing
> +/**
> + * @name: Name from a wwnn/wwpn lookup
> + *
> + * Validate that the @name fetched from the wwnn/wwpn is a vHBA
> + * and not an HBA as that should be a configuration error. It's only
> + * possible to use an existing wwnn/wwpn of a vHBA because that's
> + * what someone would have created using the node device create via XML
> + * functionality. Using the HBA "just because" it has a wwnn/wwpn and
> + * the characteristics of a vHBA is just not valid
> + *
> + * Returns true if the @name is OK, false on error
> + */
> +static bool
> +checkName(const char *name)
> +{
> +unsigned int host_num;
> +
> +if (virSCSIHostGetNumber(name, &host_num) &&
I expressed my doubts about ^this if it fails...I was wrong, if that fails,
we've got other problems, IOW virVHBAGetHostByWWN always must feed us a valid
name, since it's fetching it from the filesystem, so we should not have any
problem parsing it, thus extracting @host_num from it.
So, I'm sorry I didn't see it right away, ACK.
Erik
> +virVHBAIsVportCapable(NULL, host_num))
> +return true;
> +
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("the wwnn/wwpn for '%s' are assigned to an HBA"), name);
> +return false;
> +}
> +
> +
> /*
> * Using the host# name found via wwnn/wwpn lookup in the fc_host
> * sysfs tree to get the parent 'scsi_host#' to ensure it matches.
> @@ -288,6 +315,9 @@ createVport(virConnectPtr conn,
> * this pool and we don't have to create the vHBA
> */
> if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
> +if (!(checkName(name)))
> +goto cleanup;
> +
> /* If a parent was provided, let's make sure the 'name' we've
> * retrieved has the same parent. If not this will cause failure. */
> if (!fchost->parent || checkParent(conn, name, fchost->parent))
> --
> 2.9.4
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 0/2] Restrict usage of unsupported clock timers
hpet and kvm.pit clock timers are specific to x86 architecture and are not to be used by unsuported architectures. Similarly sPAPR guests only allow RTC timer. This patchset will restrict the usage of unsupported clock timers. Kothapally Madhu Pavan (2): qemu: Restrict usage of hpet and kvm.pit timers by unsupported architectures qemu: Default hwclock source for sPAPR to RTC src/qemu/qemu_domain.c | 24 1 file changed, 24 insertions(+) -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] storage: Disallow usage of the HBA for a fc_host backing
> One would hope it doesn't fail...
Even if it does, virSCSIHostGetNumber does report an error already.
>
> Your suggestions make checkName much cleaner:
>
> if (virSCSIHostGetNumber(name, &host_num) &&
> virVHBAIsVportCapable(NULL, host_num))
> return true;
>
if virSCSIHostGetNumber fails, it will report an error on its own but you'll
also format the one below which wouldn't be true, because at that point you
still don't know whether wwnn/wwpn refer to either an HBA or a vHBA
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>_("the wwnn/wwpn for '%s' are assigned to an HBA"),
> name);
> return false;
>
> and only add :
>
> if (!(checkName(name)))
> goto cleanup
>
> to the createVport
>
> The @name to @scsi_host_name can return to checkParent. I retest and
I'm fine with ^this change.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable
On 07/25/2017 04:25 PM, Michal Privoznik wrote:
>
> Moreover, now I can do the following and the code still works:
>
> diff --git i/src/conf/virnetworkobj.c w/src/conf/virnetworkobj.c
> index ccde72e72..4fe13fc40 100644
> --- i/src/conf/virnetworkobj.c
> +++ w/src/conf/virnetworkobj.c
> @@ -60 +60 @@ virNetworkObjOnceInit(void)
> -if (!(virNetworkObjListClass = virClassNew(virClassForObjectLockable(),
> +if (!(virNetworkObjListClass = virClassNew(virClassForObjectRWLockable(),
> diff --git i/src/conf/virnetworkobj.h w/src/conf/virnetworkobj.h
> index 8090c2e24..ee4a939f2 100644
> --- i/src/conf/virnetworkobj.h
> +++ w/src/conf/virnetworkobj.h
> @@ -30 +30 @@ struct _virNetworkObj {
> -virObjectLockable parent;
> +virObjectRWLockable parent;
>
Hit 'Send' too soon. This should have been:
diff --git i/src/conf/virnetworkobj.c w/src/conf/virnetworkobj.c
index ccde72e72..82be62832 100644
--- i/src/conf/virnetworkobj.c
+++ w/src/conf/virnetworkobj.c
@@ -41 +41 @@ struct _virNetworkObjList {
-virObjectLockable parent;
+virObjectRWLockable parent;
@@ -60 +60 @@ virNetworkObjOnceInit(void)
-if (!(virNetworkObjListClass = virClassNew(virClassForObjectLockable(),
+if (!(virNetworkObjListClass = virClassNew(virClassForObjectRWLockable(),
Obviously, rewriting virNetworkObj to use RW locks is gonna require some more
work.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable
On 07/25/2017 02:13 PM, John Ferlan wrote:
>
>
> On 07/25/2017 03:45 AM, Michal Privoznik wrote:
>> On 07/24/2017 05:22 PM, John Ferlan wrote:
>>> [...]
>>>
/**
* virObjectLock:
- * @anyobj: any instance of virObjectLockablePtr
+ * @anyobj: any instance of virObjectLockable or virObjectRWLockable
*
- * Acquire a lock on @anyobj. The lock must be
- * released by virObjectUnlock.
+ * Acquire a lock on @anyobj. The lock must be released by
+ * virObjectUnlock. In case the passed object is instance of
+ * virObjectRWLockable a write lock is acquired.
*
* The caller is expected to have acquired a reference
* on the object before locking it (eg virObjectRef).
@@ -340,31 +382,69 @@ virObjectGetLockableObj(void *anyobj)
void
virObjectLock(void *anyobj)
{
-virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
+if (virObjectIsClass(anyobj, virObjectLockableClass)) {
+virObjectLockablePtr obj = anyobj;
+virMutexLock(&obj->lock);
+} else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
+virObjectRWLockablePtr obj = anyobj;
+virRWLockWrite(&obj->lock);
+} else {
+virObjectPtr obj = anyobj;
+VIR_WARN("Object %p (%s) is not a virObjectLockable "
+ "nor virObjectRWLockable instance",
+ anyobj, obj ? obj->klass->name : "(unknown)");
+}
+}
-if (!obj)
-return;
-virMutexLock(&obj->lock);
+/**
+ * virObjectLockRead:
+ * @anyobj: any instance of virObjectRWLockablePtr
+ *
+ * Acquire a read lock on @anyobj. The lock must be
+ * released by virObjectUnlock.
+ *
+ * The caller is expected to have acquired a reference
+ * on the object before locking it (eg virObjectRef).
+ * The object must be unlocked before releasing this
+ * reference.
+ */
+void
+virObjectLockRead(void *anyobj)
+{
+if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
+virObjectRWLockablePtr obj = anyobj;
+virRWLockRead(&obj->lock);
+} else {
+virObjectPtr obj = anyobj;
+VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
+ anyobj, obj ? obj->klass->name : "(unknown)");
+}
}
>>>
>>> Hopefully no one "confuses" which object is which or no one starts using
>>> virObjectLockRead for a virObjectLockable and expects that read lock to
>>> be in place (or error out) or gasp actually wait for a write lock to
>>> release the lock as this does not error out.
>>
>> This could have already happened if one would pass bare virObject to
>> virObjectLock().
>>
>
> Consider passing a non virObject (such as what happened during an early
> change to the code for me - a virHashTablePtr) to virObjectLock...
Well, that's what happens in C to functions accepting void pointer. In
this sense yes, we are not true OOP - compiler would catch that you're
calling a method that is not defined for the class. But since we're
implementing OOP in runtime, we are able to catch OOP related problem
only at runtime. I'm not sure it is something we can check for at
compile time. And I think other C libraries that re-implement OOP (like
glib) do just the same as us.
>
> In any case, we'll have to be more vigilant now to know that only
> ObjList @obj's for virdomainobjlist can use this new API.
Why? Maybe I'm misunderstanding what you're saying, but I think that
network driver, and basically any other driver which uses vir*ObjList
can use it. And not just lists. Other objects too - e.g. qemuCaps,
virDomainCaps, etc (I really roughly skimmed through users of
virObjectLockable).
And @obj's are untouched with this change, aren't they? I feel like
we're not on the same page here. My change just allowed two threads to
look up domains at the same time. It doesn't affect domains in any way.
> Longer term
> I'll adjust to use them.
>
>>>
>>> This is a danger in the long term assumption of having a void function
>>> that has callers that can make the assumption that upon return the Lock
>>> really was taken.
>>
>> Well, I agree that this is one more thing to be cautious about, but no
>> more than making sure object passed to virObjectLock() is virObjectLockable.
>>
>
> There are some ugly ways to handle this including using some kind of
> macro for virObjectLock that would force an abort of some sort. We have
> been "fortunate" to have well behaved and reviewed code, but with two
> lock API's now it's just one of those things to keep track of for reviews.
I don't find this change that frightening, but I agree that bare
VIR_WARN() we have might not be enough. There are plenty ways where
malicious pointers can be passed to virObject* APIs, and especially
virObjectLock(). Wors
[libvirt] [PATCH v3] storage: Disallow usage of the HBA for a fc_host backing
Disallow providing the wwnn/wwpn of the HBA in the adapter XML:
This should be considered a configuration error since a vHBA
would not be created. In order to use the HBA as the backing the
following XML should be used:
So add a check prior to the checkParent call to validate that
the provided wwnn/wwpn resolves to a vHBA and not an HBA.
Signed-off-by: John Ferlan
---
v2: https://www.redhat.com/archives/libvir-list/2017-July/msg01019.html
Changes since v2, from review, simplify logic even more.
docs/formatstorage.html.in | 27 +++
src/storage/storage_backend_scsi.c | 30 ++
2 files changed, 45 insertions(+), 12 deletions(-)
diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
index 4946ddf..27578e8 100644
--- a/docs/formatstorage.html.in
+++ b/docs/formatstorage.html.in
@@ -207,18 +207,21 @@
wwnn and wwpn
- The "World Wide Node Name" (wwnn) and "World Wide
-Port Name" (wwpn) are used by the "fc_host" adapter
-to uniquely identify the device in the Fibre Channel storage fabric
-(the device can be either a HBA or vHBA). Both wwnn and wwpn should
-be specified. Use the command 'virsh nodedev-dumpxml' to determine
-how to set the values for the wwnn/wwpn of a (v)HBA. The wwnn and
-wwpn have very specific numerical format requirements based on the
-hypervisor being used, thus care should be taken if you decide to
-generate your own to follow the standards; otherwise, the pool
-will fail to start with an opaque error message indicating failure
-to write to the vport_create file during vport create/delete due
-to "No such file or directory".
+ The required "World Wide Node Name" (wwnn) and
+"World Wide Port Name" (wwpn) are used by the
+"fc_host" adapter to uniquely identify the vHBA device in the
+Fibre Channel storage fabric. If the vHBA device already exists
+as a Node Device, then libvirt will use it; otherwise, the vHBA
+will be created using the provided values. It is considered a
+configuration error use the values from the HBA as those would
+be for a "scsi_host" type pool instead. The
+wwnn and wwpn have very specific
+format requirements based on the hypervisor being used, thus
+care should be taken if you decide to generate your own to
+follow the standards; otherwise, the pool will fail to start
+with an opaque error message indicating failure to write to
+the vport_create file during vport create/delete due to
+"No such file or directory".
Since 1.0.4
diff --git a/src/storage/storage_backend_scsi.c
b/src/storage/storage_backend_scsi.c
index af12889..575e6a6 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -211,6 +211,33 @@ getAdapterName(virStorageAdapterPtr adapter)
}
+/**
+ * @name: Name from a wwnn/wwpn lookup
+ *
+ * Validate that the @name fetched from the wwnn/wwpn is a vHBA
+ * and not an HBA as that should be a configuration error. It's only
+ * possible to use an existing wwnn/wwpn of a vHBA because that's
+ * what someone would have created using the node device create via XML
+ * functionality. Using the HBA "just because" it has a wwnn/wwpn and
+ * the characteristics of a vHBA is just not valid
+ *
+ * Returns true if the @name is OK, false on error
+ */
+static bool
+checkName(const char *name)
+{
+unsigned int host_num;
+
+if (virSCSIHostGetNumber(name, &host_num) &&
+virVHBAIsVportCapable(NULL, host_num))
+return true;
+
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("the wwnn/wwpn for '%s' are assigned to an HBA"), name);
+return false;
+}
+
+
/*
* Using the host# name found via wwnn/wwpn lookup in the fc_host
* sysfs tree to get the parent 'scsi_host#' to ensure it matches.
@@ -288,6 +315,9 @@ createVport(virConnectPtr conn,
* this pool and we don't have to create the vHBA
*/
if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
+if (!(checkName(name)))
+goto cleanup;
+
/* If a parent was provided, let's make sure the 'name' we've
* retrieved has the same parent. If not this will cause failure. */
if (!fchost->parent || checkParent(conn, name, fchost->parent))
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] storage: Disallow usage of the HBA for a fc_host backing
On 07/25/2017 08:49 AM, Erik Skultety wrote:
> On Tue, Jul 25, 2017 at 07:36:48AM -0400, John Ferlan wrote:
>>
>>
>> On 07/25/2017 03:45 AM, Erik Skultety wrote:
+/**
+ * @name: Name from a wwnn/wwpn lookup
+ *
+ * Validate that the @name fetched from the wwnn/wwpn is a vHBA and not
+ * an HBA as that should be a configuration error. It's only possible to
+ * use an existing wwnn/wwpn of a vHBA because that's what someone would
+ * have created using the node device create functionality. Using the
+ * HBA "just because" it has a wwnn/wwpn and the characteristics of a
+ * vHBA is just not valid
+ *
+ * Returns the @scsi_host_name to be used or NULL on errror
>
> s/errror/error
>
> ...
>
OK
@@ -275,6 +316,7 @@ createVport(virConnectPtr conn,
virStorageAdapterFCHostPtr fchost)
{
char *name = NULL;
+char *scsi_host_name = NULL;
virStoragePoolFCRefreshInfoPtr cbdata = NULL;
virThread thread;
int ret = -1;
@@ -288,6 +330,9 @@ createVport(virConnectPtr conn,
* this pool and we don't have to create the vHBA
*/
if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
+if (!(scsi_host_name = checkName(name)))
+goto cleanup;
>>>
>>> Ok, so I learn from my mistakes, do you plan on building upon this in the
>>> foreseeable future with a follow-up series you haven't sent yet, but have
>>> on a
>>> local branch? Because if not, I don't really see a point in returning a
>>> string
>>> which only gets free'd on the function exit - checkName should probably
>>> become
>>> something like "isVHBA" and return boolean. And even if you do have a
>>> follow-up
>>> on a local branch, I still think that converting the return value should be
>>> part of that series, solely because until then, @scsi_host_name wouldn't
>>> have
>>> any meaning.
>>>
>>> Erik
>>>
>>
>> No this is it - I want to stop thinking about NPIV for a while... I
>> returned the 'scsi_host_name' string because in my mind I had passed it
>> to checkParent, but apparently I didn't do that, sigh.
>>
>> Does that make more sense now?
>
> Indeed, to be honest I missed the connection between name and scsi_host_name
> and thought, yeah ok makes sense (probably have seen quite a lot NPIV lately
> myself)now that I see it a bit more clearly, it still left me wondering if
> it wouldn't make more sense to move the scsi_host_name formatting part to
> createVport (you free it here after all) right after you pass @name to
That's fine...
> checkName/isVHBA (whatever we settle on):
> - you don't need to format it prior to your checkName, since backwards
> compatibility takes care of eating "hostX" nicely
> - you also don't need to report any error when virSCSIHostGetNumber fails,
> since one gets formatted already and it didn't bring much value
One would hope it doesn't fail...
Your suggestions make checkName much cleaner:
if (virSCSIHostGetNumber(name, &host_num) &&
virVHBAIsVportCapable(NULL, host_num))
return true;
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("the wwnn/wwpn for '%s' are assigned to an HBA"),
name);
return false;
and only add :
if (!(checkName(name)))
goto cleanup
to the createVport
The @name to @scsi_host_name can return to checkParent. I retest and
repost shortly.
Tks -
John
>
> -> then, right after that call you actually format the "scsi_" name since you
> really need it to traverse the list of devices and find the parent in
> checkParent.
>
> Erik
>
>>
>> John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] qemu: Save qemu driver in qemuDomainObjPrivateData
On 07/25/2017 03:48 PM, Martin Kletzander wrote:
> On Tue, Jul 25, 2017 at 03:20:48PM +0200, Michal Privoznik wrote:
>> On 07/24/2017 04:09 PM, Martin Kletzander wrote:
>>> This way we can finally make it static and not use any externs anywhere.
>>>
>>> Signed-off-by: Martin Kletzander
>>> ---
>>> src/qemu/qemu_domain.c | 3 ++-
>>> src/qemu/qemu_domain.h | 2 ++
>>> src/qemu/qemu_driver.c | 2 +-
>>> src/qemu/qemu_process.c | 5 +
>>> 4 files changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>>> index f1e144d92b64..0b7c45280321 100644
>>> --- a/src/qemu/qemu_domain.c
>>> +++ b/src/qemu/qemu_domain.c
>>> @@ -1662,7 +1662,7 @@ qemuDomainClearPrivatePaths(virDomainObjPtr vm)
>>>
>>>
>>> static void *
>>> -qemuDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED)
>>> +qemuDomainObjPrivateAlloc(void *opaque)
>>> {
>>> qemuDomainObjPrivatePtr priv;
>>>
>>> @@ -1679,6 +1679,7 @@ qemuDomainObjPrivateAlloc(void *opaque
>>> ATTRIBUTE_UNUSED)
>>> goto error;
>>>
>>> priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX;
>>> +priv->driver = opaque;
>>>
>>> return priv;
>>>
>>> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
>>> index 365b23c96167..71567af034f5 100644
>>> --- a/src/qemu/qemu_domain.h
>>> +++ b/src/qemu/qemu_domain.h
>>> @@ -217,6 +217,8 @@ struct _qemuDomainSecretInfo {
>>> typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate;
>>> typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr;
>>> struct _qemuDomainObjPrivate {
>>> +virQEMUDriverPtr driver;
>>> +
>>> struct qemuDomainJobObj job;
>>>
>>> virBitmapPtr namespaces;
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index 6568def156f4..9c54571cf078 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -159,7 +159,7 @@ static int qemuGetDHCPInterfaces(virDomainPtr dom,
>>> virDomainObjPtr vm,
>>> virDomainInterfacePtr **ifaces);
>>>
>>> -virQEMUDriverPtr qemu_driver = NULL;
>>> +static virQEMUDriverPtr qemu_driver;
>>>
>>>
>>> static void
>>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>>> index 525521aaf0ca..757f5d95e0b7 100644
>>> --- a/src/qemu/qemu_process.c
>>> +++ b/src/qemu/qemu_process.c
>>> @@ -120,9 +120,6 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr
>>> driver,
>>> }
>>>
>>>
>>> -/* XXX figure out how to remove this */
>>> -extern virQEMUDriverPtr qemu_driver;
>>> -
>>> /*
>>> * This is a callback registered with a qemuAgentPtr instance,
>>> * and to be invoked when the agent console hits an end of file
>>> @@ -518,9 +515,9 @@ qemuProcessHandleReset(qemuMonitorPtr mon
>>> ATTRIBUTE_UNUSED,
>>> static void
>>> qemuProcessFakeReboot(void *opaque)
>>> {
>>> -virQEMUDriverPtr driver = qemu_driver;
>>> virDomainObjPtr vm = opaque;
>>> qemuDomainObjPrivatePtr priv = vm->privateData;
>>> +virQEMUDriverPtr driver = priv->driver;
>>
>> Well, storing driver in private data looks like overkill for this
>> purpose. qemuProcessFakeReboot() runs in a thread (which explains weird
>> function arguments). But in order to pass qemu driver into this function
>> we can make the function take a struct.
>> On the other hand, it might come handy (even right now) as it enables us
>> to clean up some code where we already have both priv and driver in
>> function arguments. Frankly, I'm torn. So it's up to you whether you
>> want to go this way or the one I'm suggesting.
>>
>
> I'm perfectly fine with passing the struct and I have no problem with
> changing this that way. The clean up of qemuProcessFakeReboot is not the
> main purpose of this clean up; it's the last patch and enabling future
> clean ups. However, I have thought about it and when I pass the struct,
> it will eventually still get removed and it will end up in this form
> during the future clean ups. In other words, I can pass the struct, but
> it's not needed any more since the driver will be available even without
> that =)
>
> So I'll stick with this since you left me decide ;)
Right. We don't need both approaches. ACK to this and to 1/4 too then.
Although, let me just point out that it was difficult for me to track
that it is indeed driver that is passed as @opaque to
qemuDomainObjPrivateAlloc(). It isn't visible at the first glance.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] qemu: Save qemu driver in qemuDomainObjPrivateData
On Tue, Jul 25, 2017 at 03:20:48PM +0200, Michal Privoznik wrote:
On 07/24/2017 04:09 PM, Martin Kletzander wrote:
This way we can finally make it static and not use any externs anywhere.
Signed-off-by: Martin Kletzander
---
src/qemu/qemu_domain.c | 3 ++-
src/qemu/qemu_domain.h | 2 ++
src/qemu/qemu_driver.c | 2 +-
src/qemu/qemu_process.c | 5 +
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f1e144d92b64..0b7c45280321 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1662,7 +1662,7 @@ qemuDomainClearPrivatePaths(virDomainObjPtr vm)
static void *
-qemuDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED)
+qemuDomainObjPrivateAlloc(void *opaque)
{
qemuDomainObjPrivatePtr priv;
@@ -1679,6 +1679,7 @@ qemuDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED)
goto error;
priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX;
+priv->driver = opaque;
return priv;
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 365b23c96167..71567af034f5 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -217,6 +217,8 @@ struct _qemuDomainSecretInfo {
typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate;
typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr;
struct _qemuDomainObjPrivate {
+virQEMUDriverPtr driver;
+
struct qemuDomainJobObj job;
virBitmapPtr namespaces;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6568def156f4..9c54571cf078 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -159,7 +159,7 @@ static int qemuGetDHCPInterfaces(virDomainPtr dom,
virDomainObjPtr vm,
virDomainInterfacePtr **ifaces);
-virQEMUDriverPtr qemu_driver = NULL;
+static virQEMUDriverPtr qemu_driver;
static void
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 525521aaf0ca..757f5d95e0b7 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -120,9 +120,6 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver,
}
-/* XXX figure out how to remove this */
-extern virQEMUDriverPtr qemu_driver;
-
/*
* This is a callback registered with a qemuAgentPtr instance,
* and to be invoked when the agent console hits an end of file
@@ -518,9 +515,9 @@ qemuProcessHandleReset(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
static void
qemuProcessFakeReboot(void *opaque)
{
-virQEMUDriverPtr driver = qemu_driver;
virDomainObjPtr vm = opaque;
qemuDomainObjPrivatePtr priv = vm->privateData;
+virQEMUDriverPtr driver = priv->driver;
Well, storing driver in private data looks like overkill for this
purpose. qemuProcessFakeReboot() runs in a thread (which explains weird
function arguments). But in order to pass qemu driver into this function
we can make the function take a struct.
On the other hand, it might come handy (even right now) as it enables us
to clean up some code where we already have both priv and driver in
function arguments. Frankly, I'm torn. So it's up to you whether you
want to go this way or the one I'm suggesting.
I'm perfectly fine with passing the struct and I have no problem with
changing this that way. The clean up of qemuProcessFakeReboot is not the
main purpose of this clean up; it's the last patch and enabling future
clean ups. However, I have thought about it and when I pass the struct,
it will eventually still get removed and it will end up in this form
during the future clean ups. In other words, I can pass the struct, but
it's not needed any more since the driver will be available even without
that =)
So I'll stick with this since you left me decide ;)
Michal
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 0/3] More virsecretobj changes to prepare for common object
On Tue, Jul 25, 2017 at 09:21:52AM -0400, John Ferlan wrote: > v2: https://www.redhat.com/archives/libvir-list/2017-July/msg00520.html > > Changes in v3: > > Former patch 1, dropped > Former patch 2, pushed > Former patch 3, adjusted: > -> Use @objDef instead of @objdef > -> Handle the if (backup) leak shown during review > Former patch 4, split > -> Patch 3 handles the virsecretobj call to virSecretObjListRemove > -> Patch 4 handles the secret_driver calls to virSecretObjListRemove > > John Ferlan (3): > secret: Properly handle @def after virSecretObjAdd in driver > secret: Fix memory leak in virSecretLoad > secret: Handle object list removal and deletion properly Reviewed-by: Pavel Hrdina signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 3/3] secret: Handle object list removal and deletion properly
Rather than rely on virSecretObjEndAPI to make the final virObjectUnref
after the call to virSecretObjListRemove, be more explicit by calling
virObjectUnref and setting @obj to NULL for secretUndefine and in
the error path of secretDefineXML. Calling EndAPI will end up calling
Unlock on an already unlocked object which has indeteriminate results
(usually an ignored error).
The virSecretObjEndAPI will both Unref and Unlock the object; however,
the virSecretObjListRemove would have already Unlock'd the object so
calling Unlock again is incorrect. Once the virSecretObjListRemove
is called all that's left is to Unref our interest since that's the
corrollary to the virSecretObjListAdd which returned our ref interest
plus references for each hash table in which the object resides. In math
terms, after an Add there's 2 refs on the object (1 for the object and
1 for the list). After calling Remove there's just 1 ref on the object.
For the Add callers, calling EndAPI removes the ref for the object and
unlocks it, but since it's in a list the other 1 remains.
Signed-off-by: John Ferlan
---
src/secret/secret_driver.c | 4
1 file changed, 4 insertions(+)
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index 8defa46..d833a86 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -270,6 +270,8 @@ secretDefineXML(virConnectPtr conn,
VIR_STEAL_PTR(def, objDef);
} else {
virSecretObjListRemove(driver->secrets, obj);
+virObjectUnref(obj);
+obj = NULL;
}
cleanup:
@@ -410,6 +412,8 @@ secretUndefine(virSecretPtr secret)
virSecretObjDeleteData(obj);
virSecretObjListRemove(driver->secrets, obj);
+virObjectUnref(obj);
+obj = NULL;
ret = 0;
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 2/3] secret: Fix memory leak in virSecretLoad
If the virSecretLoadValue fails, the code jumped to cleanup without
setting @ret = obj, thus calling virSecretObjListRemove which only
accounts for the object reference related to adding the object to
the list during virSecretObjListAdd, but does not account for the
reference to the object itself as the return of @ret would be NULL
so the caller wouldn't call virSecretObjEndAPI on the object recently
added thus reducing the refcnt to zero.
This patch will perform the ObjListRemove in the failure path of
virSecretLoadValue and Unref @obj in order to perform clean up
and return @obj as NULL. The @def will be freed as part of the
virObjectUnref.
Signed-off-by: John Ferlan
---
src/conf/virsecretobj.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index 850fdaf..aa994ba 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -914,7 +914,6 @@ virSecretLoad(virSecretObjListPtr secrets,
{
virSecretDefPtr def = NULL;
virSecretObjPtr obj = NULL;
-virSecretObjPtr ret = NULL;
if (!(def = virSecretDefParseFile(path)))
goto cleanup;
@@ -926,16 +925,15 @@ virSecretLoad(virSecretObjListPtr secrets,
goto cleanup;
def = NULL;
-if (virSecretLoadValue(obj) < 0)
-goto cleanup;
-
-ret = obj;
-obj = NULL;
+if (virSecretLoadValue(obj) < 0) {
+virSecretObjListRemove(secrets, obj);
+virObjectUnref(obj);
+obj = NULL;
+}
cleanup:
-virSecretObjListRemove(secrets, obj);
virSecretDefFree(def);
-return ret;
+return obj;
}
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 1/3] secret: Properly handle @def after virSecretObjAdd in driver
Since the virSecretObjListAdd technically consumes @def on success,
the secretDefineXML should set @def = NULL immediately and process
the remaining calls using a new @objDef variable. We can use use
VIR_STEAL_PTR since we know the Add function just stores @def in
obj->def.
Because we steal @def into @objDef, if we jump to restore_backup:
and @backup is set, then we need to ensure the @def would be
free'd properly, so we'll steal it back from @objDef. For the other
condition this fixes a double free of @def if the code had jumped to
@backup == NULL thus calling virSecretObjListRemove without setting
@def = NULL. In this case, the subsequent call to DefFree would
succeed and free @def; however, the call to EndAPI would also
call DefFree because the Unref done would be the last one for
the @obj meaning the obj->def would be used to call DefFree,
but it's already been free'd because @def wasn't managed right
within this error path.
Signed-off-by: John Ferlan
---
src/secret/secret_driver.c | 27 ++-
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index 30124b4..8defa46 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -210,6 +210,7 @@ secretDefineXML(virConnectPtr conn,
{
virSecretPtr ret = NULL;
virSecretObjPtr obj = NULL;
+virSecretDefPtr objDef;
virSecretDefPtr backup = NULL;
virSecretDefPtr def;
virObjectEventPtr event = NULL;
@@ -225,8 +226,9 @@ secretDefineXML(virConnectPtr conn,
if (!(obj = virSecretObjListAdd(driver->secrets, def,
driver->configDir, &backup)))
goto cleanup;
+VIR_STEAL_PTR(objDef, def);
-if (!def->isephemeral) {
+if (!objDef->isephemeral) {
if (backup && backup->isephemeral) {
if (virSecretObjSaveData(obj) < 0)
goto restore_backup;
@@ -248,28 +250,27 @@ secretDefineXML(virConnectPtr conn,
/* Saved successfully - drop old values */
virSecretDefFree(backup);
-event = virSecretEventLifecycleNew(def->uuid,
- def->usage_type,
- def->usage_id,
+event = virSecretEventLifecycleNew(objDef->uuid,
+ objDef->usage_type,
+ objDef->usage_id,
VIR_SECRET_EVENT_DEFINED,
0);
ret = virGetSecret(conn,
- def->uuid,
- def->usage_type,
- def->usage_id);
-def = NULL;
+ objDef->uuid,
+ objDef->usage_type,
+ objDef->usage_id);
goto cleanup;
restore_backup:
/* If we have a backup, then secret was defined before, so just restore
- * the backup. The current def will be handled below.
- * Otherwise, this is a new secret, thus remove it.
- */
-if (backup)
+ * the backup; otherwise, this is a new secret, thus remove it. */
+if (backup) {
virSecretObjSetDef(obj, backup);
-else
+VIR_STEAL_PTR(def, objDef);
+} else {
virSecretObjListRemove(driver->secrets, obj);
+}
cleanup:
virSecretDefFree(def);
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 0/3] More virsecretobj changes to prepare for common object
v2: https://www.redhat.com/archives/libvir-list/2017-July/msg00520.html Changes in v3: Former patch 1, dropped Former patch 2, pushed Former patch 3, adjusted: -> Use @objDef instead of @objdef -> Handle the if (backup) leak shown during review Former patch 4, split -> Patch 3 handles the virsecretobj call to virSecretObjListRemove -> Patch 4 handles the secret_driver calls to virSecretObjListRemove John Ferlan (3): secret: Properly handle @def after virSecretObjAdd in driver secret: Fix memory leak in virSecretLoad secret: Handle object list removal and deletion properly src/conf/virsecretobj.c| 14 ++ src/secret/secret_driver.c | 31 ++- 2 files changed, 24 insertions(+), 21 deletions(-) -- 2.9.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] qemu: Save qemu driver in qemuDomainObjPrivateData
On 07/24/2017 04:09 PM, Martin Kletzander wrote:
> This way we can finally make it static and not use any externs anywhere.
>
> Signed-off-by: Martin Kletzander
> ---
> src/qemu/qemu_domain.c | 3 ++-
> src/qemu/qemu_domain.h | 2 ++
> src/qemu/qemu_driver.c | 2 +-
> src/qemu/qemu_process.c | 5 +
> 4 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index f1e144d92b64..0b7c45280321 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1662,7 +1662,7 @@ qemuDomainClearPrivatePaths(virDomainObjPtr vm)
>
>
> static void *
> -qemuDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED)
> +qemuDomainObjPrivateAlloc(void *opaque)
> {
> qemuDomainObjPrivatePtr priv;
>
> @@ -1679,6 +1679,7 @@ qemuDomainObjPrivateAlloc(void *opaque ATTRIBUTE_UNUSED)
> goto error;
>
> priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX;
> +priv->driver = opaque;
>
> return priv;
>
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 365b23c96167..71567af034f5 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -217,6 +217,8 @@ struct _qemuDomainSecretInfo {
> typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate;
> typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr;
> struct _qemuDomainObjPrivate {
> +virQEMUDriverPtr driver;
> +
> struct qemuDomainJobObj job;
>
> virBitmapPtr namespaces;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 6568def156f4..9c54571cf078 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -159,7 +159,7 @@ static int qemuGetDHCPInterfaces(virDomainPtr dom,
> virDomainObjPtr vm,
> virDomainInterfacePtr **ifaces);
>
> -virQEMUDriverPtr qemu_driver = NULL;
> +static virQEMUDriverPtr qemu_driver;
>
>
> static void
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 525521aaf0ca..757f5d95e0b7 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -120,9 +120,6 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver,
> }
>
>
> -/* XXX figure out how to remove this */
> -extern virQEMUDriverPtr qemu_driver;
> -
> /*
> * This is a callback registered with a qemuAgentPtr instance,
> * and to be invoked when the agent console hits an end of file
> @@ -518,9 +515,9 @@ qemuProcessHandleReset(qemuMonitorPtr mon
> ATTRIBUTE_UNUSED,
> static void
> qemuProcessFakeReboot(void *opaque)
> {
> -virQEMUDriverPtr driver = qemu_driver;
> virDomainObjPtr vm = opaque;
> qemuDomainObjPrivatePtr priv = vm->privateData;
> +virQEMUDriverPtr driver = priv->driver;
Well, storing driver in private data looks like overkill for this
purpose. qemuProcessFakeReboot() runs in a thread (which explains weird
function arguments). But in order to pass qemu driver into this function
we can make the function take a struct.
On the other hand, it might come handy (even right now) as it enables us
to clean up some code where we already have both priv and driver in
function arguments. Frankly, I'm torn. So it's up to you whether you
want to go this way or the one I'm suggesting.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/4] lxc: Make lxcProcessStop callable even without PID being available
On 07/24/2017 04:09 PM, Martin Kletzander wrote: > This way the function can work as a central point of clean-up code and > we don't have to duplicate code. And it works similarly to the qemu > driver. > > Signed-off-by: Martin Kletzander > --- > src/lxc/lxc_process.c | 32 ++-- > 1 file changed, 2 insertions(+), 30 deletions(-) ACK Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] Move machineName generation from virsystemd into domain_conf
On 07/24/2017 04:09 PM, Martin Kletzander wrote: > It is more related to a domain as we might use it even when there is > no systemd and it does not use any dbus/systemd functions. In order > not to use code from conf/ in util/ pass machineName in cgroups code > as a parameter. That also fixes a leak of machineName in the lxc > driver and cleans up and de-duplicates some code. > > Signed-off-by: Martin Kletzander > --- > src/conf/domain_conf.c | 62 > > src/conf/domain_conf.h | 5 > src/libvirt_private.syms | 2 +- > src/lxc/lxc_cgroup.c | 5 +--- > src/lxc/lxc_domain.c | 19 +++ > src/lxc/lxc_domain.h | 3 +++ > src/lxc/lxc_process.c| 25 +-- > src/qemu/qemu_cgroup.c | 24 --- > src/qemu/qemu_domain.c | 21 > src/qemu/qemu_domain.h | 3 +++ > src/qemu/qemu_process.c | 6 + > src/util/vircgroup.c | 15 > src/util/vircgroup.h | 14 +-- > src/util/virsystemd.c| 62 > > src/util/virsystemd.h| 5 > tests/virsystemdtest.c | 4 ++-- > 16 files changed, 153 insertions(+), 122 deletions(-) ACK Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/4] secret: Properly handle @def after virSecretObjAdd in driver
On Tue, Jul 25, 2017 at 08:37:23AM -0400, John Ferlan wrote:
>
>
> On 07/25/2017 08:21 AM, Pavel Hrdina wrote:
> > On Tue, Jul 25, 2017 at 01:36:20PM +0200, Pavel Hrdina wrote:
> >> On Fri, Jul 14, 2017 at 10:04:41AM -0400, John Ferlan wrote:
> >>> Since the virSecretObjListAdd technically consumes @def on success,
> >>> the secretDefineXML should set @def = NULL immediately and process
> >>> the remaining calls using a new @objdef variable. We can use use
> >>> VIR_STEAL_PTR since we know the Add function just stores @def in
> >>> obj->def.
> >>>
> >>> This fixes a possible double free of @def if the code jumps to
> >>> restore_backup: and calls virSecretObjListRemove without setting
> >>> def = NULL. In this case, the subsequent call to DefFree would
> >>> succeed and free @def; however, the call to EndAPI would also
> >>> call DefFree because the Unref done would be the last one for
> >>> the @obj meaning the obj->def would be used to call DefFree,
> >>> but it's already been free'd because @def wasn't managed right
> >>> within this error path.
> >>>
> >>> Signed-off-by: John Ferlan
> >>> ---
> >>> src/secret/secret_driver.c | 19 ++-
> >>> 1 file changed, 10 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
> >>> index 30124b4..77351d8 100644
> >>> --- a/src/secret/secret_driver.c
> >>> +++ b/src/secret/secret_driver.c
> >>> @@ -210,6 +210,7 @@ secretDefineXML(virConnectPtr conn,
> >>> {
> >>> virSecretPtr ret = NULL;
> >>> virSecretObjPtr obj = NULL;
> >>> +virSecretDefPtr objdef;
> >>
> >> s/objdef/objDef/
> >>
> >> Reviewed-by: Pavel Hrdina
> >
> > Actually this patch introduces a memory leak in case we restore the
> > @backup definition.
> >
> > virSecretObjListAdd() adds the @def into @obj and sets the old def
> > into @backup, so far good.
> >
> > VIR_STEAL_PTR(objdef, def) leaves the @def == NULL, still good
> >
> > But if we in any point jump to "restore_backup" after the VIR_STEAL_PTR
> > and the @backup != NULL we simply changes the @obj->def back to @backup
> > and the new def is only in @objdef which is not freed anywhere.
>
> I went through this in the previous review, but for the opposite
> condition where backup == NULL (repeated for ease)
>
> Without the stealing of objdef, when return from the Add function we
> have "refcnt=2" and "lock=true".
>
> For some reason we jump to restore_backup and call ObjListRemove which
> returns and the object refcnt=1 and lock=false.
>
> We fall into cleanup:
>
> Call virSecretDefFree(def) where @def != NULL, so it free's it
> Call virSecretObjEndAPI(&obj) which calls Unref, setting R=0 and calling
> virSecretDefFree(obj->def)... But wait, we already did that because we
> allowed the DefFree(def) to free something it didn't own.
>
>
> >
> > Possible fix is to set @def = NULL after the virSecretObjListRemove()
> > in restore_backup:
> >
> > ...
> > } else {
> > virSecretObjListRemove(driver->secrets, obj);
> > virObjectUnref(obj);
> > obj = NULL;
> > def = NULL;
> > }
> > ...
> >
>
> The other fix to the leak you point out is:
>
> if (backup) {
> virSecretObjSetDef(obj, backup);
> def = objdef;
> } else {
> virSecretObjListRemove(driver->secrets, obj);
> }
That works for me, but still I would like to use objDef.
Pavel
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] storage: Disallow usage of the HBA for a fc_host backing
On Tue, Jul 25, 2017 at 07:36:48AM -0400, John Ferlan wrote:
>
>
> On 07/25/2017 03:45 AM, Erik Skultety wrote:
> >> +/**
> >> + * @name: Name from a wwnn/wwpn lookup
> >> + *
> >> + * Validate that the @name fetched from the wwnn/wwpn is a vHBA and not
> >> + * an HBA as that should be a configuration error. It's only possible to
> >> + * use an existing wwnn/wwpn of a vHBA because that's what someone would
> >> + * have created using the node device create functionality. Using the
> >> + * HBA "just because" it has a wwnn/wwpn and the characteristics of a
> >> + * vHBA is just not valid
> >> + *
> >> + * Returns the @scsi_host_name to be used or NULL on errror
s/errror/error
...
> >> @@ -275,6 +316,7 @@ createVport(virConnectPtr conn,
> >> virStorageAdapterFCHostPtr fchost)
> >> {
> >> char *name = NULL;
> >> +char *scsi_host_name = NULL;
> >> virStoragePoolFCRefreshInfoPtr cbdata = NULL;
> >> virThread thread;
> >> int ret = -1;
> >> @@ -288,6 +330,9 @@ createVport(virConnectPtr conn,
> >> * this pool and we don't have to create the vHBA
> >> */
> >> if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
> >> +if (!(scsi_host_name = checkName(name)))
> >> +goto cleanup;
> >
> > Ok, so I learn from my mistakes, do you plan on building upon this in the
> > foreseeable future with a follow-up series you haven't sent yet, but have
> > on a
> > local branch? Because if not, I don't really see a point in returning a
> > string
> > which only gets free'd on the function exit - checkName should probably
> > become
> > something like "isVHBA" and return boolean. And even if you do have a
> > follow-up
> > on a local branch, I still think that converting the return value should be
> > part of that series, solely because until then, @scsi_host_name wouldn't
> > have
> > any meaning.
> >
> > Erik
> >
>
> No this is it - I want to stop thinking about NPIV for a while... I
> returned the 'scsi_host_name' string because in my mind I had passed it
> to checkParent, but apparently I didn't do that, sigh.
>
> Does that make more sense now?
Indeed, to be honest I missed the connection between name and scsi_host_name
and thought, yeah ok makes sense (probably have seen quite a lot NPIV lately
myself)now that I see it a bit more clearly, it still left me wondering if
it wouldn't make more sense to move the scsi_host_name formatting part to
createVport (you free it here after all) right after you pass @name to
checkName/isVHBA (whatever we settle on):
- you don't need to format it prior to your checkName, since backwards
compatibility takes care of eating "hostX" nicely
- you also don't need to report any error when virSCSIHostGetNumber fails,
since one gets formatted already and it didn't bring much value
-> then, right after that call you actually format the "scsi_" name since you
really need it to traverse the list of devices and find the parent in
checkParent.
Erik
>
> John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/4] secret: Properly handle @def after virSecretObjAdd in driver
On 07/25/2017 08:21 AM, Pavel Hrdina wrote:
> On Tue, Jul 25, 2017 at 01:36:20PM +0200, Pavel Hrdina wrote:
>> On Fri, Jul 14, 2017 at 10:04:41AM -0400, John Ferlan wrote:
>>> Since the virSecretObjListAdd technically consumes @def on success,
>>> the secretDefineXML should set @def = NULL immediately and process
>>> the remaining calls using a new @objdef variable. We can use use
>>> VIR_STEAL_PTR since we know the Add function just stores @def in
>>> obj->def.
>>>
>>> This fixes a possible double free of @def if the code jumps to
>>> restore_backup: and calls virSecretObjListRemove without setting
>>> def = NULL. In this case, the subsequent call to DefFree would
>>> succeed and free @def; however, the call to EndAPI would also
>>> call DefFree because the Unref done would be the last one for
>>> the @obj meaning the obj->def would be used to call DefFree,
>>> but it's already been free'd because @def wasn't managed right
>>> within this error path.
>>>
>>> Signed-off-by: John Ferlan
>>> ---
>>> src/secret/secret_driver.c | 19 ++-
>>> 1 file changed, 10 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
>>> index 30124b4..77351d8 100644
>>> --- a/src/secret/secret_driver.c
>>> +++ b/src/secret/secret_driver.c
>>> @@ -210,6 +210,7 @@ secretDefineXML(virConnectPtr conn,
>>> {
>>> virSecretPtr ret = NULL;
>>> virSecretObjPtr obj = NULL;
>>> +virSecretDefPtr objdef;
>>
>> s/objdef/objDef/
>>
>> Reviewed-by: Pavel Hrdina
>
> Actually this patch introduces a memory leak in case we restore the
> @backup definition.
>
> virSecretObjListAdd() adds the @def into @obj and sets the old def
> into @backup, so far good.
>
> VIR_STEAL_PTR(objdef, def) leaves the @def == NULL, still good
>
> But if we in any point jump to "restore_backup" after the VIR_STEAL_PTR
> and the @backup != NULL we simply changes the @obj->def back to @backup
> and the new def is only in @objdef which is not freed anywhere.
I went through this in the previous review, but for the opposite
condition where backup == NULL (repeated for ease)
Without the stealing of objdef, when return from the Add function we
have "refcnt=2" and "lock=true".
For some reason we jump to restore_backup and call ObjListRemove which
returns and the object refcnt=1 and lock=false.
We fall into cleanup:
Call virSecretDefFree(def) where @def != NULL, so it free's it
Call virSecretObjEndAPI(&obj) which calls Unref, setting R=0 and calling
virSecretDefFree(obj->def)... But wait, we already did that because we
allowed the DefFree(def) to free something it didn't own.
>
> Possible fix is to set @def = NULL after the virSecretObjListRemove()
> in restore_backup:
>
> ...
> } else {
> virSecretObjListRemove(driver->secrets, obj);
> virObjectUnref(obj);
> obj = NULL;
> def = NULL;
> }
> ...
>
The other fix to the leak you point out is:
if (backup) {
virSecretObjSetDef(obj, backup);
def = objdef;
} else {
virSecretObjListRemove(driver->secrets, obj);
}
> and there is no need to have yet another @objdef.
Unless you want the other condition of having two free's of @def
>
> Pavel
>
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/4] secret: Properly handle @def after virSecretObjAdd in driver
On Tue, Jul 25, 2017 at 08:23:04AM -0400, John Ferlan wrote:
>
>
> On 07/25/2017 07:36 AM, Pavel Hrdina wrote:
> > On Fri, Jul 14, 2017 at 10:04:41AM -0400, John Ferlan wrote:
> >> Since the virSecretObjListAdd technically consumes @def on success,
> >> the secretDefineXML should set @def = NULL immediately and process
> >> the remaining calls using a new @objdef variable. We can use use
> >> VIR_STEAL_PTR since we know the Add function just stores @def in
> >> obj->def.
> >>
> >> This fixes a possible double free of @def if the code jumps to
> >> restore_backup: and calls virSecretObjListRemove without setting
> >> def = NULL. In this case, the subsequent call to DefFree would
> >> succeed and free @def; however, the call to EndAPI would also
> >> call DefFree because the Unref done would be the last one for
> >> the @obj meaning the obj->def would be used to call DefFree,
> >> but it's already been free'd because @def wasn't managed right
> >> within this error path.
> >>
> >> Signed-off-by: John Ferlan
> >> ---
> >> src/secret/secret_driver.c | 19 ++-
> >> 1 file changed, 10 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
> >> index 30124b4..77351d8 100644
> >> --- a/src/secret/secret_driver.c
> >> +++ b/src/secret/secret_driver.c
> >> @@ -210,6 +210,7 @@ secretDefineXML(virConnectPtr conn,
> >> {
> >> virSecretPtr ret = NULL;
> >> virSecretObjPtr obj = NULL;
> >> +virSecretDefPtr objdef;
> >
> > s/objdef/objDef/
>
> Why? I've been using objdef in general and not the camel case one
Well, the naming convention is usually a camelCase or snake_case.
In that case other usages of objdef are not correct as well. Yes in
this case it's easy to distinguish the two parts on the variable name,
but I still thing that camelCase is the preferred form.
Pavel
>
> John
> >
> > Reviewed-by: Pavel Hrdina
> >
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/4] secret: Properly handle @def after virSecretObjAdd in driver
On 07/25/2017 07:36 AM, Pavel Hrdina wrote:
> On Fri, Jul 14, 2017 at 10:04:41AM -0400, John Ferlan wrote:
>> Since the virSecretObjListAdd technically consumes @def on success,
>> the secretDefineXML should set @def = NULL immediately and process
>> the remaining calls using a new @objdef variable. We can use use
>> VIR_STEAL_PTR since we know the Add function just stores @def in
>> obj->def.
>>
>> This fixes a possible double free of @def if the code jumps to
>> restore_backup: and calls virSecretObjListRemove without setting
>> def = NULL. In this case, the subsequent call to DefFree would
>> succeed and free @def; however, the call to EndAPI would also
>> call DefFree because the Unref done would be the last one for
>> the @obj meaning the obj->def would be used to call DefFree,
>> but it's already been free'd because @def wasn't managed right
>> within this error path.
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/secret/secret_driver.c | 19 ++-
>> 1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
>> index 30124b4..77351d8 100644
>> --- a/src/secret/secret_driver.c
>> +++ b/src/secret/secret_driver.c
>> @@ -210,6 +210,7 @@ secretDefineXML(virConnectPtr conn,
>> {
>> virSecretPtr ret = NULL;
>> virSecretObjPtr obj = NULL;
>> +virSecretDefPtr objdef;
>
> s/objdef/objDef/
Why? I've been using objdef in general and not the camel case one
John
>
> Reviewed-by: Pavel Hrdina
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/4] secret: Properly handle @def after virSecretObjAdd in driver
On Tue, Jul 25, 2017 at 01:36:20PM +0200, Pavel Hrdina wrote:
> On Fri, Jul 14, 2017 at 10:04:41AM -0400, John Ferlan wrote:
> > Since the virSecretObjListAdd technically consumes @def on success,
> > the secretDefineXML should set @def = NULL immediately and process
> > the remaining calls using a new @objdef variable. We can use use
> > VIR_STEAL_PTR since we know the Add function just stores @def in
> > obj->def.
> >
> > This fixes a possible double free of @def if the code jumps to
> > restore_backup: and calls virSecretObjListRemove without setting
> > def = NULL. In this case, the subsequent call to DefFree would
> > succeed and free @def; however, the call to EndAPI would also
> > call DefFree because the Unref done would be the last one for
> > the @obj meaning the obj->def would be used to call DefFree,
> > but it's already been free'd because @def wasn't managed right
> > within this error path.
> >
> > Signed-off-by: John Ferlan
> > ---
> > src/secret/secret_driver.c | 19 ++-
> > 1 file changed, 10 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
> > index 30124b4..77351d8 100644
> > --- a/src/secret/secret_driver.c
> > +++ b/src/secret/secret_driver.c
> > @@ -210,6 +210,7 @@ secretDefineXML(virConnectPtr conn,
> > {
> > virSecretPtr ret = NULL;
> > virSecretObjPtr obj = NULL;
> > +virSecretDefPtr objdef;
>
> s/objdef/objDef/
>
> Reviewed-by: Pavel Hrdina
Actually this patch introduces a memory leak in case we restore the
@backup definition.
virSecretObjListAdd() adds the @def into @obj and sets the old def
into @backup, so far good.
VIR_STEAL_PTR(objdef, def) leaves the @def == NULL, still good
But if we in any point jump to "restore_backup" after the VIR_STEAL_PTR
and the @backup != NULL we simply changes the @obj->def back to @backup
and the new def is only in @objdef which is not freed anywhere.
Possible fix is to set @def = NULL after the virSecretObjListRemove()
in restore_backup:
...
} else {
virSecretObjListRemove(driver->secrets, obj);
virObjectUnref(obj);
obj = NULL;
def = NULL;
}
...
and there is no need to have yet another @objdef.
Pavel
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] secret: Clean up virSecretObjListAdd processing
On 07/25/2017 07:21 AM, Pavel Hrdina wrote:
> On Fri, Jul 14, 2017 at 10:04:39AM -0400, John Ferlan wrote:
>> Make use of an error: label to handle the failure and need to call
>> virSecretObjEndAPI for the object to set it to NULL for return.
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/conf/virsecretobj.c | 22 +++---
>> 1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
>> index e3bcbe5..bedcdbd 100644
>> --- a/src/conf/virsecretobj.c
>> +++ b/src/conf/virsecretobj.c
>> @@ -333,7 +333,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>> {
>> virSecretObjPtr obj;
>> virSecretDefPtr objdef;
>> -virSecretObjPtr ret = NULL;
>> char uuidstr[VIR_UUID_STRING_BUFLEN];
>> char *configFile = NULL, *base64File = NULL;
>>
>> @@ -354,13 +353,13 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>> _("a secret with UUID %s is already defined for "
>> "use with %s"),
>> uuidstr, objdef->usage_id);
>> -goto cleanup;
>> +goto error;
>> }
>>
>> if (objdef->isprivate && !newdef->isprivate) {
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> _("cannot change private flag on existing
>> secret"));
>> -goto cleanup;
>> +goto error;
>> }
>>
>> if (oldDef)
>> @@ -369,8 +368,9 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>> virSecretDefFree(objdef);
>> obj->def = newdef;
>> } else {
>> +
>> /* No existing secret with same UUID,
>> - * try look for matching usage instead */
>> + * try to look for matching usage instead */
>> if ((obj = virSecretObjListFindByUsageLocked(secrets,
>> newdef->usage_type,
>> newdef->usage_id))) {
>> @@ -381,7 +381,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>> _("a secret with UUID %s already defined for "
>> "use with %s"),
>> uuidstr, newdef->usage_id);
>> -goto cleanup;
>> +goto error;
>> }
>>
>> /* Generate the possible configFile and base64File strings
>> @@ -395,7 +395,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>> goto cleanup;
>>
>> if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
>> -goto cleanup;
>> +goto error;
>>
>> obj->def = newdef;
>> VIR_STEAL_PTR(obj->configFile, configFile);
>> @@ -403,15 +403,15 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>> virObjectRef(obj);
>> }
>>
>> -ret = obj;
>> -obj = NULL;
>> -
>> cleanup:
>> -virSecretObjEndAPI(&obj);
>> VIR_FREE(configFile);
>> VIR_FREE(base64File);
>> virObjectUnlock(secrets);
>> -return ret;
>> +return obj;
>> +
>> + error:
>> +virSecretObjEndAPI(&obj);
>> +goto cleanup;
>
> I don't see what's wrong with the current code, it's commonly used
> pattern to steal the pointer. The extra error label would make sense
> if the error path would be more complex, but in this case it doesn't
> add any value.
>
> Pavel
>
Fine - I'll drop this one then.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable
On 07/25/2017 03:45 AM, Michal Privoznik wrote:
> On 07/24/2017 05:22 PM, John Ferlan wrote:
>> [...]
>>
>>> /**
>>> * virObjectLock:
>>> - * @anyobj: any instance of virObjectLockablePtr
>>> + * @anyobj: any instance of virObjectLockable or virObjectRWLockable
>>> *
>>> - * Acquire a lock on @anyobj. The lock must be
>>> - * released by virObjectUnlock.
>>> + * Acquire a lock on @anyobj. The lock must be released by
>>> + * virObjectUnlock. In case the passed object is instance of
>>> + * virObjectRWLockable a write lock is acquired.
>>> *
>>> * The caller is expected to have acquired a reference
>>> * on the object before locking it (eg virObjectRef).
>>> @@ -340,31 +382,69 @@ virObjectGetLockableObj(void *anyobj)
>>> void
>>> virObjectLock(void *anyobj)
>>> {
>>> -virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
>>> +if (virObjectIsClass(anyobj, virObjectLockableClass)) {
>>> +virObjectLockablePtr obj = anyobj;
>>> +virMutexLock(&obj->lock);
>>> +} else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
>>> +virObjectRWLockablePtr obj = anyobj;
>>> +virRWLockWrite(&obj->lock);
>>> +} else {
>>> +virObjectPtr obj = anyobj;
>>> +VIR_WARN("Object %p (%s) is not a virObjectLockable "
>>> + "nor virObjectRWLockable instance",
>>> + anyobj, obj ? obj->klass->name : "(unknown)");
>>> +}
>>> +}
>>>
>>> -if (!obj)
>>> -return;
>>>
>>> -virMutexLock(&obj->lock);
>>> +/**
>>> + * virObjectLockRead:
>>> + * @anyobj: any instance of virObjectRWLockablePtr
>>> + *
>>> + * Acquire a read lock on @anyobj. The lock must be
>>> + * released by virObjectUnlock.
>>> + *
>>> + * The caller is expected to have acquired a reference
>>> + * on the object before locking it (eg virObjectRef).
>>> + * The object must be unlocked before releasing this
>>> + * reference.
>>> + */
>>> +void
>>> +virObjectLockRead(void *anyobj)
>>> +{
>>> +if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
>>> +virObjectRWLockablePtr obj = anyobj;
>>> +virRWLockRead(&obj->lock);
>>> +} else {
>>> +virObjectPtr obj = anyobj;
>>> +VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
>>> + anyobj, obj ? obj->klass->name : "(unknown)");
>>> +}
>>> }
>>>
>>
>> Hopefully no one "confuses" which object is which or no one starts using
>> virObjectLockRead for a virObjectLockable and expects that read lock to
>> be in place (or error out) or gasp actually wait for a write lock to
>> release the lock as this does not error out.
>
> This could have already happened if one would pass bare virObject to
> virObjectLock().
>
Consider passing a non virObject (such as what happened during an early
change to the code for me - a virHashTablePtr) to virObjectLock...
In any case, we'll have to be more vigilant now to know that only
ObjList @obj's for virdomainobjlist can use this new API. Longer term
I'll adjust to use them.
>>
>> This is a danger in the long term assumption of having a void function
>> that has callers that can make the assumption that upon return the Lock
>> really was taken.
>
> Well, I agree that this is one more thing to be cautious about, but no
> more than making sure object passed to virObjectLock() is virObjectLockable.
>
There are some ugly ways to handle this including using some kind of
macro for virObjectLock that would force an abort of some sort. We have
been "fortunate" to have well behaved and reviewed code, but with two
lock API's now it's just one of those things to keep track of for reviews.
>>
>> Perhaps the better way to attack this would have been to create a
>> virObjectLockWrite() function called only from vir*ObjListAdd and
>> vir*ObjListRemove leaving the other virObjectLock()'s in tact and having
>> the virObjectLock() code make the decision over whether to use the
>> virRWLockRead or virMutexLock depending on the object class.
>
> What's the difference? If virObjectLock() does what you're suggesting
> (what indeed is implemented too), what's the point in having
> virObjectLockWrite()?
>
> Michal
>
1. Less code and a lock that could check status... I understand not
wanting to modify 10 callers to check status, but modifying two and thus
making it clearer to the caller that these are "different" might not be
a bad thing.
2. The "default" action being more consumers probably will use the Read
locks (as I believe is the premise/impetus of the series). The
Add/Remove would seemingly be used less and thus the exception. So why
not have the default for ObjList/virObjectRWLockableClass be to have the
virObjectLock use a virRWLockRead instead of virRWLockWrite?
What's done is done - I obviously have ulterior motives to not wanting
virobject to change that much, but I've been considering the downside of
the code that has void functions that really don't return a lock on the
object if the
[libvirt] [PATCH glib] Don't set LC_ALL=C during build as that breaks python apps
Setting LC_ALL=C breaks python apps doing I/O on UTF-8 source files. In particular this broke glib-mkenums GEN libvirt-gconfig-enum-types.h Traceback (most recent call last): File "/usr/bin/glib-mkenums", line 669, in process_file(fname) File "/usr/bin/glib-mkenums", line 406, in process_file line = curfile.readline() File "/usr/lib64/python3.6/encodings/ascii.py", line 26, in decode return codecs.ascii_decode(input, self.errors)[0] UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 849: ordinal not in range(128) Signed-off-by: Daniel P. Berrange --- Pushed to fix rawhide build maint.mk | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/maint.mk b/maint.mk index 405c6d0..ef72b4f 100644 --- a/maint.mk +++ b/maint.mk @@ -117,8 +117,8 @@ news-check-lines-spec ?= 1,10 news-check-regexp ?= '^\*.* $(VERSION_REGEXP) \($(today)\)' # Prevent programs like 'sort' from considering distinct strings to be equal. -# Doing it here saves us from having to set LC_ALL elsewhere in this file. -export LC_ALL = C +# Doing it here saves us from having to set LC_COLLATE elsewhere in this file. +export LC_COLLATE = C ## --- ## ## Sanity checks. ## -- 2.13.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 4/4] secret: Handle object list removal and deletion properly
On Fri, Jul 14, 2017 at 10:04:42AM -0400, John Ferlan wrote:
> Rather than rely on virSecretObjEndAPI to make the final virObjectUnref
> after the call to virSecretObjListRemove, be more explicit by calling
> virObjectUnref and setting @obj to NULL for secretUndefine and in
> the error path of secretDefineXML. Calling EndAPI will end up calling
> Unlock on an already unlocked object which has indeteriminate results
> (usually an ignored error).
>
> The virSecretObjEndAPI will both Unref and Unlock the object; however,
> the virSecretObjListRemove would have already Unlock'd the object so
> calling Unlock again is incorrect. Once the virSecretObjListRemove
> is called all that's left is to Unref our interest since that's the
> corrollary to the virSecretObjListAdd which returned our ref interest
> plus references for each hash table in which the object resides. In math
> terms, after an Add there's 2 refs on the object (1 for the object and
> 1 for the list). After calling Remove there's just 1 ref on the object.
> For the Add callers, calling EndAPI removes the ref for the object and
> unlocks it, but since it's in a list the other 1 remains.
>
> This also fixes a leak during virSecretLoad if the virSecretLoadValue
> fails the code jumps to cleanup without setting @ret = obj, thus calling
> virSecretObjListRemove which only accounts for the object reference
> related to adding the object to the list during virSecretObjListAdd,
> but does not account for the reference to the object itself as the
> return of @ret would be NULL so the caller wouldn't call virSecretObjEndAPI
> on the object recently added thus reducing the refcnt to zero. Thus
> cleaning up the virSecretLoadValue error path to make it clearer what
> needs to be done on failure.
>
> Signed-off-by: John Ferlan
> ---
> src/conf/virsecretobj.c| 14 ++
> src/secret/secret_driver.c | 9 +++--
> 2 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
> index dd36ce6..0e7675e 100644
> --- a/src/conf/virsecretobj.c
> +++ b/src/conf/virsecretobj.c
> @@ -914,7 +914,6 @@ virSecretLoad(virSecretObjListPtr secrets,
> {
> virSecretDefPtr def = NULL;
> virSecretObjPtr obj = NULL;
> -virSecretObjPtr ret = NULL;
>
> if (!(def = virSecretDefParseFile(path)))
> goto cleanup;
> @@ -926,16 +925,15 @@ virSecretLoad(virSecretObjListPtr secrets,
> goto cleanup;
> def = NULL;
>
> -if (virSecretLoadValue(obj) < 0)
> -goto cleanup;
> -
> -ret = obj;
> -obj = NULL;
> +if (virSecretLoadValue(obj) < 0) {
> +virSecretObjListRemove(secrets, obj);
> +virObjectUnref(obj);
> +obj = NULL;
> +}
>
> cleanup:
> -virSecretObjListRemove(secrets, obj);
> virSecretDefFree(def);
> -return ret;
> +return obj;
> }
This should be split into two separate patches, the first part that
fixes virSecretLoad() address memory leak and the second part for
secretDefineXML() and secretUndefine() fixes a double unlock.
Pavel
>
>
> diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
> index 77351d8..19ba6fd 100644
> --- a/src/secret/secret_driver.c
> +++ b/src/secret/secret_driver.c
> @@ -267,10 +267,13 @@ secretDefineXML(virConnectPtr conn,
> * the backup. The current def will be Free'd below.
> * Otherwise, this is a new secret, thus remove it.
> */
> -if (backup)
> +if (backup) {
> virSecretObjSetDef(obj, backup);
> -else
> +} else {
> virSecretObjListRemove(driver->secrets, obj);
> +virObjectUnref(obj);
> +obj = NULL;
> +}
>
> cleanup:
> virSecretDefFree(def);
> @@ -410,6 +413,8 @@ secretUndefine(virSecretPtr secret)
> virSecretObjDeleteData(obj);
>
> virSecretObjListRemove(driver->secrets, obj);
> +virObjectUnref(obj);
> +obj = NULL;
>
> ret = 0;
>
> --
> 2.9.4
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] storage: Disallow usage of the HBA for a fc_host backing
On 07/25/2017 03:45 AM, Erik Skultety wrote:
>> +/**
>> + * @name: Name from a wwnn/wwpn lookup
>> + *
>> + * Validate that the @name fetched from the wwnn/wwpn is a vHBA and not
>> + * an HBA as that should be a configuration error. It's only possible to
>> + * use an existing wwnn/wwpn of a vHBA because that's what someone would
>> + * have created using the node device create functionality. Using the
>> + * HBA "just because" it has a wwnn/wwpn and the characteristics of a
>> + * vHBA is just not valid
>> + *
>> + * Returns the @scsi_host_name to be used or NULL on errror
>> + */
>> +static char *
>> +checkName(const char *name)
>> +{
>> +char *scsi_host_name = NULL;
>> +unsigned int host_num;
>> +
>> +if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
>> +return NULL;
>> +
>> +if (virSCSIHostGetNumber(scsi_host_name, &host_num) < 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("host name '%s' is not properly formatted"),
>> + name);
>> +goto error;
>> +}
>> +
>> +/* If scsi_host_name is vport capable, then it's an HBA. This is
>> + * a configuration error as the wwnn/wwpn should only be for a vHBA */
>> +if (virVHBAIsVportCapable(NULL, host_num)) {
>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("the wwnn/wwpn for '%s' are assigned to an HBA"),
>> + scsi_host_name);
>> +goto error;
>> +}
>> +
>> +return scsi_host_name;
>> +
>> + error:
>> +VIR_FREE(scsi_host_name);
>> +return NULL;
>> +}
>> +
>
> ...
>
>> @@ -275,6 +316,7 @@ createVport(virConnectPtr conn,
>> virStorageAdapterFCHostPtr fchost)
>> {
>> char *name = NULL;
>> +char *scsi_host_name = NULL;
>> virStoragePoolFCRefreshInfoPtr cbdata = NULL;
>> virThread thread;
>> int ret = -1;
>> @@ -288,6 +330,9 @@ createVport(virConnectPtr conn,
>> * this pool and we don't have to create the vHBA
>> */
>> if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
>> +if (!(scsi_host_name = checkName(name)))
>> +goto cleanup;
>
> Ok, so I learn from my mistakes, do you plan on building upon this in the
> foreseeable future with a follow-up series you haven't sent yet, but have on a
> local branch? Because if not, I don't really see a point in returning a string
> which only gets free'd on the function exit - checkName should probably become
> something like "isVHBA" and return boolean. And even if you do have a
> follow-up
> on a local branch, I still think that converting the return value should be
> part of that series, solely because until then, @scsi_host_name wouldn't have
> any meaning.
>
> Erik
>
No this is it - I want to stop thinking about NPIV for a while... I
returned the 'scsi_host_name' string because in my mind I had passed it
to checkParent, but apparently I didn't do that, sigh.
Does that make more sense now?
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/4] secret: Properly handle @def after virSecretObjAdd in driver
On Fri, Jul 14, 2017 at 10:04:41AM -0400, John Ferlan wrote:
> Since the virSecretObjListAdd technically consumes @def on success,
> the secretDefineXML should set @def = NULL immediately and process
> the remaining calls using a new @objdef variable. We can use use
> VIR_STEAL_PTR since we know the Add function just stores @def in
> obj->def.
>
> This fixes a possible double free of @def if the code jumps to
> restore_backup: and calls virSecretObjListRemove without setting
> def = NULL. In this case, the subsequent call to DefFree would
> succeed and free @def; however, the call to EndAPI would also
> call DefFree because the Unref done would be the last one for
> the @obj meaning the obj->def would be used to call DefFree,
> but it's already been free'd because @def wasn't managed right
> within this error path.
>
> Signed-off-by: John Ferlan
> ---
> src/secret/secret_driver.c | 19 ++-
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
> index 30124b4..77351d8 100644
> --- a/src/secret/secret_driver.c
> +++ b/src/secret/secret_driver.c
> @@ -210,6 +210,7 @@ secretDefineXML(virConnectPtr conn,
> {
> virSecretPtr ret = NULL;
> virSecretObjPtr obj = NULL;
> +virSecretDefPtr objdef;
s/objdef/objDef/
Reviewed-by: Pavel Hrdina
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/4] secret: Remove need for local configFile and base64File in ObjectAdd
On Fri, Jul 14, 2017 at 10:04:40AM -0400, John Ferlan wrote: > Rather than assign to a local variable, let's just assign directly to the > object using the error path for cleanup. > > Signed-off-by: John Ferlan > --- > src/conf/virsecretobj.c | 17 ++--- > 1 file changed, 6 insertions(+), 11 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] secret: Clean up virSecretObjListAdd processing
On Fri, Jul 14, 2017 at 10:04:39AM -0400, John Ferlan wrote:
> Make use of an error: label to handle the failure and need to call
> virSecretObjEndAPI for the object to set it to NULL for return.
>
> Signed-off-by: John Ferlan
> ---
> src/conf/virsecretobj.c | 22 +++---
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
> index e3bcbe5..bedcdbd 100644
> --- a/src/conf/virsecretobj.c
> +++ b/src/conf/virsecretobj.c
> @@ -333,7 +333,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
> {
> virSecretObjPtr obj;
> virSecretDefPtr objdef;
> -virSecretObjPtr ret = NULL;
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> char *configFile = NULL, *base64File = NULL;
>
> @@ -354,13 +353,13 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
> _("a secret with UUID %s is already defined for "
> "use with %s"),
> uuidstr, objdef->usage_id);
> -goto cleanup;
> +goto error;
> }
>
> if (objdef->isprivate && !newdef->isprivate) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("cannot change private flag on existing
> secret"));
> -goto cleanup;
> +goto error;
> }
>
> if (oldDef)
> @@ -369,8 +368,9 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
> virSecretDefFree(objdef);
> obj->def = newdef;
> } else {
> +
> /* No existing secret with same UUID,
> - * try look for matching usage instead */
> + * try to look for matching usage instead */
> if ((obj = virSecretObjListFindByUsageLocked(secrets,
> newdef->usage_type,
> newdef->usage_id))) {
> @@ -381,7 +381,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
> _("a secret with UUID %s already defined for "
> "use with %s"),
> uuidstr, newdef->usage_id);
> -goto cleanup;
> +goto error;
> }
>
> /* Generate the possible configFile and base64File strings
> @@ -395,7 +395,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
> goto cleanup;
>
> if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
> -goto cleanup;
> +goto error;
>
> obj->def = newdef;
> VIR_STEAL_PTR(obj->configFile, configFile);
> @@ -403,15 +403,15 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
> virObjectRef(obj);
> }
>
> -ret = obj;
> -obj = NULL;
> -
> cleanup:
> -virSecretObjEndAPI(&obj);
> VIR_FREE(configFile);
> VIR_FREE(base64File);
> virObjectUnlock(secrets);
> -return ret;
> +return obj;
> +
> + error:
> +virSecretObjEndAPI(&obj);
> +goto cleanup;
I don't see what's wrong with the current code, it's commonly used
pattern to steal the pointer. The extra error label would make sense
if the error path would be more complex, but in this case it doesn't
add any value.
Pavel
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] docs: Add "PCI topology and hotplug" guidelines
For all machine types except i440fx, making a guest hotplug capable requires some sort of planning. Add some information to help users make educated choices when defining the PCI topology of guests. Signed-off-by: Andrea Bolognani --- docs/formatdomain.html.in | 4 +- docs/pci-hotplug.html.in | 164 ++ 2 files changed, 167 insertions(+), 1 deletion(-) create mode 100644 docs/pci-hotplug.html.in diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index bceddd2..7c4450c 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3505,7 +3505,9 @@ appear more than once, with a group of virtual devices tied to a virtual controller. Normally, libvirt can automatically infer such controllers without requiring explicit XML markup, but sometimes - it is necessary to provide an explicit controller element. + it is necessary to provide an explicit controller element, notably + when planning the PCI topology + for guests where device hotplug is expected. diff --git a/docs/pci-hotplug.html.in b/docs/pci-hotplug.html.in new file mode 100644 index 000..f3d1610 --- /dev/null +++ b/docs/pci-hotplug.html.in @@ -0,0 +1,164 @@ + +http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd";> +http://www.w3.org/1999/xhtml";> + +PCI topology and hotplug + + + + + Perhaps surprisingly, most libvirt guests support only limited PCI + device hotplug out of the box, or even none at all. + + + The reason for this apparent limitation is the fact that each + hotplugged PCI device might require additional PCI controllers to + be added to the guest, and libvirt has no way of knowing in advance + how many devices will be hotplugged during the guest's lifetime, + thus making it impossible to automatically provide the right amount + of PCI controllers: any arbitrary number would end up being too big + for some users, and too small for others. + + + Ultimately, the user is the only one who knows how much the guest + will need to grow dynamically, so the responsability of planning + a suitabile PCI topology in advance falls on them. + + + This document aims at providing all the information needed to + successfully plan the PCI topology of a guest. Note that the + details can vary a lot between architectures and even machine + types, hence the way it's organized. + + +x86_64 architecture + +q35 machine type + + + This is a PCI Express native machine type. The default PCI topology + looks like + + + ++ + + + + and supports hotplugging a single PCI Express device, either + emulated or assigned from the host. + + + Slots on the pcie-root controller do not support + hotplug, so the device will be hotplugged into the + pcie-root-port controller. If you plan to hotplug + more than a single PCI Express device, you should add a suitable + number of pcie-root-port controllers when defining + the guest: for example, add + + + ++ + + + + + + + if you expect to hotplug up to three PCI Express devices, + either emulated or assigned from the host. That's all the + information you need to provide: libvirt will fill in the + remaining details automatically. + + + If you expect to hotplug legacy PCI devices, then you will need + specialized controllers, since all those mentioned above are + intended for PCI Express devices only: add + + + + + + + + and you'll be able to hotplug up to 31 legacy PCI devices, + either emulated or assigned from the host. + + +i440fx (pc) machine type + + + This is a legacy PCI native machine type. The default PCI + topology looks like + + + + + + + where each of the 31 slots on the pci-root + controller is hotplug capable and can accept a legacy PCI + device, either emulated or assigned from the guest. + + +ppc64 architecture + +pseries machine type + + + The default PCI topology for the pseries machine + type looks like + + + + + + + + The 31 slots on a pci-root controller are all + h+ +
Re: [libvirt] [PATCH v3 00/16] virObject adjustments for common object
On Thu, Jun 22, 2017 at 10:02:30AM -0400, John Ferlan wrote:
Let's move the discussion [1] into correct place.
> Still, I must be missing something. Why is it wrong to create a new
> object that would have a specific use? virObjectLockable was created at
> some point in history and then used as a way to provide locking for a
> virObject that only had a @ref. Someone could still create a virObject
> as well and just get @refs. With the new objects based on those two
> objects you get a few more features that allow for add, search, lookup,
> remove. But you call that wrong, which is confusing to me.
What's wrong, or better wording, what I don't like personally about this
approach is that in order to have features like add, search, lookup,
remove it would require that the object is derived from
virObjectLookupKeys. The only thing it adds to the simple virObject
is two variables @uuid and @name.
We don't need the virObjectLookupKeys object at all.
For example, current add/remove API is:
virObjectLookupHashNew
virObjectLookupHashNew(virClassPtr klass,
int tableElemsStart);
could be
virObjectListNew(int tableSize,
bool useName,
bool useUUID);
or
typedef enum {
VIR_OBJECT_LIST_TABLE_NAME = (1 << 0),
VIR_OBJECT_LIST_TABLE_UUID = (1 << 1),
} virObjectListTableType;
virObjectListNew(int tableSize,
unsigned int flags);
I don't see why the virObjectLookupHashNew() has to have the @klass
parameter and the @tableElemsStart is not correct, it's not the number
of elements, it's has table size.
virObjectLookupHash(Add|Remove)
virObjectLookupHashAdd(void *tableobj,
virObjectLookupKeysPtr obj);
virObjectLookupHashRemove(void *tableobj,
virObjectLookupKeysPtr obj);
The only difference without the virObjectLookupKeys object would be
that the API will take two more parameters to give it the @uuid and
@name.
virObjectListAdd(virObjectListPtr listObj,
void *obj,
const char *name,
const char *uuid);
virObjectListRemove(virObjectListPtr listObj,
const char *name,
const char *uuid);
or
virObjectListRemoveByName(virObjectListPtr listObj,
const char *name);
virObjectListRemoveByUUID(virObjectListPtr listObj,
const char *uuid);
Yes, at first it may looks worse, but on the other hand it gives you
better flexibility in the way that the object added/removed from the
list doesn't even have to have the exact same variables and it will
work with every object that is derived from virObject.
virObjectLookupHashFind
virObjectLookupHashFind(void *tableobj,
bool useUUID,
const char *key);
could be split into two APIs:
virObjectListFindByName(virObjectListPtr listObj,
const char *name);
virObjectListFindByUUID(virObjectListPtr listObj,
const char *UUID);
Which in my opinion is way better than having a single API with a
boolean parameter that switches what type of key it is.
virObjectLookupHashForEach
virObjectLookupHashForEach(void *tableobj,
bool useUUID,
virHashIterator callback,
virObjectLookupHashForEachDataPtr data);
could be
virObjectListForEach(virObjectListPtr listObj,
virHashIterator callback,
void *callbackData);
There are two things that I don't like. There is no need to have the
@useUUID because both tables should contain the same objects if both are
used so this can be hidden from the user and the API will simply use one
of the available tables. The second issue is that this API forces you
to use some predefined data structure and you cannot create your own
specifically tailored for the task that you are about to do.
The @useUUID argument also applies to virObjectLookupHashSearch().
The usage of the virObjectList* APIs could be something like this:
virObjectListPtr
virInterfaceObjListNew(void) {
return virObjectListNew(10, VIR_OBJECT_LIST_TABLE_NAME);
}
static virInterfaceObjPtr
virInterfaceObjListFindByName(virObjectListPtr interfaces,
const char *name)
{
return virObjectListFindByName(interfaces, name);
}
Pavel
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Revert "virthread: Introduce virRWLockInitPreferWriter"
On Tue, Jul 25, 2017 at 10:43:50AM +0200, Michal Privoznik wrote: > This reverts commit 328bd24443d2a345a5832ee48ebba0208f8036ea. > > As it turns out, this is not portable and very Linux & glibc > specific. Worse, this may lead to not starving writers on Linux > but everywhere else. Revert this and if the starvation occurs > resolve it. > > Signed-off-by: Michal Privoznik Reviewed-by: Daniel P. Berrange Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Revert "virthread: Introduce virRWLockInitPreferWriter"
This reverts commit 328bd24443d2a345a5832ee48ebba0208f8036ea.
As it turns out, this is not portable and very Linux & glibc
specific. Worse, this may lead to not starving writers on Linux
but everywhere else. Revert this and if the starvation occurs
resolve it.
Signed-off-by: Michal Privoznik
---
src/libvirt_private.syms | 1 -
src/util/virobject.c | 2 +-
src/util/virthread.c | 35 ---
src/util/virthread.h | 1 -
4 files changed, 1 insertion(+), 38 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3cc6f4c6e..d98417678 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2733,7 +2733,6 @@ virMutexUnlock;
virOnce;
virRWLockDestroy;
virRWLockInit;
-virRWLockInitPreferWriter;
virRWLockRead;
virRWLockUnlock;
virRWLockWrite;
diff --git a/src/util/virobject.c b/src/util/virobject.c
index 4236abfef..b1bb378b4 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -275,7 +275,7 @@ virObjectRWLockableNew(virClassPtr klass)
if (!(obj = virObjectNew(klass)))
return NULL;
-if (virRWLockInitPreferWriter(&obj->lock) < 0) {
+if (virRWLockInit(&obj->lock) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Unable to initialize RW lock"));
virObjectUnref(obj);
diff --git a/src/util/virthread.c b/src/util/virthread.c
index a8dd72f8b..6c495158f 100644
--- a/src/util/virthread.c
+++ b/src/util/virthread.c
@@ -95,15 +95,6 @@ void virMutexUnlock(virMutexPtr m)
}
-/**
- * virRWLockInit:
- * @m: rwlock to init
- *
- * Initializes RW lock using pthread default attributes (which
- * is PTHREAD_RWLOCK_PREFER_READER_NP).
- *
- * Returns 0 on success, -1 otherwise.
- */
int virRWLockInit(virRWLockPtr m)
{
int ret;
@@ -115,32 +106,6 @@ int virRWLockInit(virRWLockPtr m)
return 0;
}
-
-/**
- * virRWLockInitPreferWriter:
- * @m: rwlock to init
- *
- * Initializes RW lock which prefers writers over readers.
- *
- * Returns 0 on success, -1 otherwise.
- */
-int virRWLockInitPreferWriter(virRWLockPtr m)
-{
-int ret;
-pthread_rwlockattr_t attr;
-
-pthread_rwlockattr_init(&attr);
-pthread_rwlockattr_setkind_np(&attr,
PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP);
-ret = pthread_rwlock_init(&m->lock, &attr);
-pthread_rwlockattr_destroy(&attr);
-if (ret != 0) {
-errno = ret;
-return -1;
-}
-return 0;
-}
-
-
void virRWLockDestroy(virRWLockPtr m)
{
pthread_rwlock_destroy(&m->lock);
diff --git a/src/util/virthread.h b/src/util/virthread.h
index 18b785af2..e466d9bf0 100644
--- a/src/util/virthread.h
+++ b/src/util/virthread.h
@@ -136,7 +136,6 @@ void virMutexUnlock(virMutexPtr m);
int virRWLockInit(virRWLockPtr m) ATTRIBUTE_RETURN_CHECK;
-int virRWLockInitPreferWriter(virRWLockPtr m) ATTRIBUTE_RETURN_CHECK;
void virRWLockDestroy(virRWLockPtr m);
void virRWLockRead(virRWLockPtr m);
--
2.13.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] virthread: Introduce virRWLockInitPreferWriter
On Mon, Jul 24, 2017 at 01:12:59PM -0400, John Ferlan wrote: > > > On 07/19/2017 10:31 AM, Michal Privoznik wrote: > > We already have virRWLockInit. But this uses pthread defaults > > which prefer reader to initialize the RW lock. This may lead to > > writer starvation. Therefore we need to have the counterpart that > > prefers writers. Now, according to the > > pthread_rwlockattr_setkind_np() man page setting > > PTHREAD_RWLOCK_PREFER_WRITER_NP attribute is no-op. Therefore we > > need to use PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP > > attribute. So much for good enum value names. > > > > Signed-off-by: Michal Privoznik > > --- > > src/libvirt_private.syms | 1 + > > src/util/virthread.c | 35 +++ > > src/util/virthread.h | 1 + > > 3 files changed, 37 insertions(+) > > > > This has broken the CI build, freebsd is not happy: > > ../../src/util/virthread.c:133:42: error: use of undeclared identifier > 'PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP' > pthread_rwlockattr_setkind_np(&attr, > PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP); > ^ > 1 error generated. It is not just FreeBSD, it also breaks OS-X and Win32. The suffix '_np' / '_NP' is shorthand for nNon portable" - these are glibc inventions. IMHO we should not really use this in our code as if we're going to make assumptions that writers are not starved as a result, because writers will be starved on any non-Linux platform. IOW, I think we need to just revert this. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Generate unique socket file
On Mon, Jul 24, 2017 at 10:34:49AM -0500, s...@us.ibm.com wrote:
From: Scott Garfinkle
No need to have a cover letter, the information from the cover letter
you sent should instead be here.
It's possible to have more than one unix channel with type=virtio-serial.
We need to generate a unique name for each channel. Currently, we use
"unknown.sock" for all of them.
The reason you get that is because you gave them no target name. The
right way to do this is to actually give them target names so that you
can differentiate them in the guest nicely. We might generate some
names for the sockets and let them get just some vportXnY identifiers in
the guest, but I would rather prefer making users name the channels.
---
src/qemu/qemu_domain.c | 23 +++---
.../qemuxml2argv-channel-virtio-unix.args | 2 +-
2 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 78e75f1..791d4cf 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7178,18 +7178,27 @@ int
qemuDomainPrepareChannel(virDomainChrDefPtr channel,
const char *domainChannelTargetDir)
{
-if (channel->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO &&
-channel->source->type == VIR_DOMAIN_CHR_TYPE_UNIX &&
-!channel->source->data.nix.path) {
+if (channel->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO ||
+channel->source->type != VIR_DOMAIN_CHR_TYPE_UNIX ||
+channel->source->data.nix.path)
+return 0;
+
+if (channel->target.name) {
if (virAsprintf(&channel->source->data.nix.path,
"%s/%s", domainChannelTargetDir,
-channel->target.name ? channel->target.name
-: "unknown.sock") < 0)
+channel->target.name) < 0)
+return -1;
+} else {// Generate a unique name
+if (virAsprintf(&channel->source->data.nix.path,
+"%s/unknown.sock%d%d%d", domainChannelTargetDir,
+channel->info.addr.vioserial.controller,
+channel->info.addr.vioserial.bus,
+channel->info.addr.vioserial.port) < 0)
This needs some separator between the numbers, also in this case I would
rename it to something else than "unknown.sock" and most of all, leave
the '.sock' at the end.
return -1;
-
-channel->source->data.nix.listen = true;
}
+channel->source->data.nix.listen = true;
+
return 0;
}
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.args
b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.args
index 2b72965..c875a09 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.args
@@ -29,7 +29,7 @@
path=/tmp/channel/domain--1-QEMUGuest1/org.qemu.guest_agent.0,server,nowait \
-device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,\
id=channel0,name=org.qemu.guest_agent.0 \
-chardev socket,id=charchannel1,\
-path=/tmp/channel/domain--1-QEMUGuest1/unknown.sock,server,nowait \
+path=/tmp/channel/domain--1-QEMUGuest1/unknown.sock002,server,nowait \
-device virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel1,\
id=channel1 \
-chardev socket,id=charchannel2,path=/tmp/channel/domain--1-QEMUGuest1/ble,\
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 0/8] Virtio-crypto device support
Ping... On 2017/7/7 16:07, Longpeng(Mike) wrote: > As virtio-crypto has been supported in QEMU 2.8 and the frontend > driver has been merged in linux 4.10, so it's necessary to support > virtio-crypto in libvirt. > > --- > Changes since v3: > - spilt the capabilities part into a separate patch. [Boris] > - include Boris's virtio-crypto ccw support(PATCH 6 & 8). [Boris] > - add the missing capabilities in caps_2.9.0.x86_64.xml. [Boris] > - fix Indentation and missing virDomainCryptoDefFree. [Marc] > > Changes since v2: > - PATCH 1: modify docs as Martin & Boris's suggestion. [Martin & Boris] > - PATCH 2: add the missing 'ToString'. [Martin] > - PATCH 3: use virAsprintf instead of virBufferAsprintf. [Martin] > remove pointless virBufferCheckError. [Martin] > - rebase on master. [Longpeng] > > Changes since v1: > - split patch [Martin] > - rebase on master [Martin] > - add docs/tests/schema [Martin] > - fix typos [Gonglei] > > --- > Boris Fiuczynski (2): > qemu: virtio-crypto: add ccw support > qemu: virtio-crypto: add test for ccw support > > Longpeng(Mike) (6): > docs: schema: Add basic documentation for the virtual > docs: news: Add virtio-crypto devices > conf: Parse virtio-crypto in the domain XML > caps: Add qemu capabilities about virtio-crypto > qemu: Implement support for 'builtin' backend for virtio-crypto > tests: Add testcase for virtio-crypto parsing > > docs/formatdomain.html.in | 61 ++ > docs/news.xml | 10 + > docs/schemas/domaincommon.rng | 30 +++ > src/conf/domain_conf.c | 213 > - > src/conf/domain_conf.h | 32 > src/libvirt_private.syms | 5 + > src/qemu/qemu_alias.c | 20 ++ > src/qemu/qemu_alias.h | 3 + > src/qemu/qemu_capabilities.c | 6 + > src/qemu/qemu_capabilities.h | 4 + > src/qemu/qemu_command.c| 130 + > src/qemu/qemu_command.h| 3 + > src/qemu/qemu_domain_address.c | 25 +++ > src/qemu/qemu_driver.c | 6 + > src/qemu/qemu_hotplug.c| 1 + > tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml| 2 + > tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml | 2 + > tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 2 + > .../qemuxml2argv-virtio-crypto-builtin.xml | 26 +++ > .../qemuxml2argv-virtio-crypto-ccw.args| 22 +++ > .../qemuxml2argv-virtio-crypto-ccw.xml | 16 ++ > .../qemuxml2argv-virtio-crypto.args| 22 +++ > tests/qemuxml2argvtest.c | 6 + > .../qemuxml2xmlout-virtio-crypto-builtin.xml | 31 +++ > tests/qemuxml2xmltest.c| 2 + > 25 files changed, 679 insertions(+), 1 deletion(-) > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-virtio-crypto-builtin.xml > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-crypto-ccw.args > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-crypto-ccw.xml > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-crypto.args > create mode 100644 > tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-crypto-builtin.xml > -- Regards, Longpeng(Mike) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] virthread: Introduce virRWLockInitPreferWriter
On 07/24/2017 07:12 PM, John Ferlan wrote: > > > On 07/19/2017 10:31 AM, Michal Privoznik wrote: >> We already have virRWLockInit. But this uses pthread defaults >> which prefer reader to initialize the RW lock. This may lead to >> writer starvation. Therefore we need to have the counterpart that >> prefers writers. Now, according to the >> pthread_rwlockattr_setkind_np() man page setting >> PTHREAD_RWLOCK_PREFER_WRITER_NP attribute is no-op. Therefore we >> need to use PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP >> attribute. So much for good enum value names. >> >> Signed-off-by: Michal Privoznik >> --- >> src/libvirt_private.syms | 1 + >> src/util/virthread.c | 35 +++ >> src/util/virthread.h | 1 + >> 3 files changed, 37 insertions(+) >> > > This has broken the CI build, freebsd is not happy: > > ../../src/util/virthread.c:133:42: error: use of undeclared identifier > 'PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP' > pthread_rwlockattr_setkind_np(&attr, > PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP); > ^ > 1 error generated. > > > John > > You know what my suggestion is ;-) Actually, I don't. I am trying to search freebsd documentation on initializing RW locks so that they prefer writers. But no luck so far. Does anybody here on the list know? Although, for the current usage of RW locks it's really hard to starve a writer. But if this is going to be used more broadly it is going to be easier. On the other hand, I just realized that while one can use mutexes + conditions (like we do for domain jobs), using RW locks and conditions is not implemented anywhere, so I will have to come up with some idea. Long story short, for now we don't care if writer will starve. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable
On 07/24/2017 05:22 PM, John Ferlan wrote:
> [...]
>
>> /**
>> * virObjectLock:
>> - * @anyobj: any instance of virObjectLockablePtr
>> + * @anyobj: any instance of virObjectLockable or virObjectRWLockable
>> *
>> - * Acquire a lock on @anyobj. The lock must be
>> - * released by virObjectUnlock.
>> + * Acquire a lock on @anyobj. The lock must be released by
>> + * virObjectUnlock. In case the passed object is instance of
>> + * virObjectRWLockable a write lock is acquired.
>> *
>> * The caller is expected to have acquired a reference
>> * on the object before locking it (eg virObjectRef).
>> @@ -340,31 +382,69 @@ virObjectGetLockableObj(void *anyobj)
>> void
>> virObjectLock(void *anyobj)
>> {
>> -virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
>> +if (virObjectIsClass(anyobj, virObjectLockableClass)) {
>> +virObjectLockablePtr obj = anyobj;
>> +virMutexLock(&obj->lock);
>> +} else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
>> +virObjectRWLockablePtr obj = anyobj;
>> +virRWLockWrite(&obj->lock);
>> +} else {
>> +virObjectPtr obj = anyobj;
>> +VIR_WARN("Object %p (%s) is not a virObjectLockable "
>> + "nor virObjectRWLockable instance",
>> + anyobj, obj ? obj->klass->name : "(unknown)");
>> +}
>> +}
>>
>> -if (!obj)
>> -return;
>>
>> -virMutexLock(&obj->lock);
>> +/**
>> + * virObjectLockRead:
>> + * @anyobj: any instance of virObjectRWLockablePtr
>> + *
>> + * Acquire a read lock on @anyobj. The lock must be
>> + * released by virObjectUnlock.
>> + *
>> + * The caller is expected to have acquired a reference
>> + * on the object before locking it (eg virObjectRef).
>> + * The object must be unlocked before releasing this
>> + * reference.
>> + */
>> +void
>> +virObjectLockRead(void *anyobj)
>> +{
>> +if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
>> +virObjectRWLockablePtr obj = anyobj;
>> +virRWLockRead(&obj->lock);
>> +} else {
>> +virObjectPtr obj = anyobj;
>> +VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
>> + anyobj, obj ? obj->klass->name : "(unknown)");
>> +}
>> }
>>
>
> Hopefully no one "confuses" which object is which or no one starts using
> virObjectLockRead for a virObjectLockable and expects that read lock to
> be in place (or error out) or gasp actually wait for a write lock to
> release the lock as this does not error out.
This could have already happened if one would pass bare virObject to
virObjectLock().
>
> This is a danger in the long term assumption of having a void function
> that has callers that can make the assumption that upon return the Lock
> really was taken.
Well, I agree that this is one more thing to be cautious about, but no
more than making sure object passed to virObjectLock() is virObjectLockable.
>
> Perhaps the better way to attack this would have been to create a
> virObjectLockWrite() function called only from vir*ObjListAdd and
> vir*ObjListRemove leaving the other virObjectLock()'s in tact and having
> the virObjectLock() code make the decision over whether to use the
> virRWLockRead or virMutexLock depending on the object class.
What's the difference? If virObjectLock() does what you're suggesting
(what indeed is implemented too), what's the point in having
virObjectLockWrite()?
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] storage: Disallow usage of the HBA for a fc_host backing
> +/**
> + * @name: Name from a wwnn/wwpn lookup
> + *
> + * Validate that the @name fetched from the wwnn/wwpn is a vHBA and not
> + * an HBA as that should be a configuration error. It's only possible to
> + * use an existing wwnn/wwpn of a vHBA because that's what someone would
> + * have created using the node device create functionality. Using the
> + * HBA "just because" it has a wwnn/wwpn and the characteristics of a
> + * vHBA is just not valid
> + *
> + * Returns the @scsi_host_name to be used or NULL on errror
> + */
> +static char *
> +checkName(const char *name)
> +{
> +char *scsi_host_name = NULL;
> +unsigned int host_num;
> +
> +if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
> +return NULL;
> +
> +if (virSCSIHostGetNumber(scsi_host_name, &host_num) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("host name '%s' is not properly formatted"),
> + name);
> +goto error;
> +}
> +
> +/* If scsi_host_name is vport capable, then it's an HBA. This is
> + * a configuration error as the wwnn/wwpn should only be for a vHBA */
> +if (virVHBAIsVportCapable(NULL, host_num)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("the wwnn/wwpn for '%s' are assigned to an HBA"),
> + scsi_host_name);
> +goto error;
> +}
> +
> +return scsi_host_name;
> +
> + error:
> +VIR_FREE(scsi_host_name);
> +return NULL;
> +}
> +
...
> @@ -275,6 +316,7 @@ createVport(virConnectPtr conn,
> virStorageAdapterFCHostPtr fchost)
> {
> char *name = NULL;
> +char *scsi_host_name = NULL;
> virStoragePoolFCRefreshInfoPtr cbdata = NULL;
> virThread thread;
> int ret = -1;
> @@ -288,6 +330,9 @@ createVport(virConnectPtr conn,
> * this pool and we don't have to create the vHBA
> */
> if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
> +if (!(scsi_host_name = checkName(name)))
> +goto cleanup;
Ok, so I learn from my mistakes, do you plan on building upon this in the
foreseeable future with a follow-up series you haven't sent yet, but have on a
local branch? Because if not, I don't really see a point in returning a string
which only gets free'd on the function exit - checkName should probably become
something like "isVHBA" and return boolean. And even if you do have a follow-up
on a local branch, I still think that converting the return value should be
part of that series, solely because until then, @scsi_host_name wouldn't have
any meaning.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
