Re: [libvirt] [PATCH-RFC] qemu: Add network bandwidth setting for ethernet interfaces
On Fri, Sep 05, 2014 at 11:42:58PM +, Anirban Chakraborty wrote:
On 9/5/14, 1:31 AM, "Martin Kletzander" wrote:
On Thu, Sep 04, 2014 at 03:02:54PM -0700, Anirban Chakraborty wrote:
ethernet interfaces in libvirt currently do not support bandwidth
setting.
For example, following xml file for an interface will not apply these
settings to corresponding qdiscs.
-
This patch fixes the behavior.
Although this doesn't confuse git, it might confuse something else.
Leaving it without '' is ok.
Please review it and if it appears ok, please apply.
thanks,
Anirban Chakraborty
No need to have this in the commit message, it's kept in the log then.
Signed-off-by: Anirban Chakraborty
---
src/qemu/qemu_command.c | 5 +
src/qemu/qemu_hotplug.c | 3 +++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2184caa..258c6a7 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7251,6 +7251,11 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
if (tapfd[0] < 0)
goto cleanup;
}
+ /* Configure network bandwidth for ethernet type network interfaces */
+ if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)
+ if (virNetDevBandwidthSet(net->ifname,
+ virDomainNetGetActualBandwidth(net), false) < 0)
+ goto cleanup;
In libvirt, we indent by spaces. This would be caught by running
'make syntax-check'. Anyway, running 'make syntax-check check' is a
good practice to follow before formatting the patches.
Got it. Thanks for the pointer.
if ((actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a364c52..aeb53c5 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -940,6 +940,9 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd,
&vhostfdSize) < 0)
goto cleanup;
} else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) {
+ if (virNetDevBandwidthSet(net->ifname,
+ virDomainNetGetActualBandwidth(net), false) < 0)
+ goto cleanup;
Same here.
We need to clear the bandwidth settings when shutting down the domain
or unplugging the device.
If I am not mistaken, currently, the bandwidth setting has been cleared
only for bridged devices (via
networkShutdownNetworkVirtual‹>virNetDevBandwidthClear ). Are you saying
that we should explicitly clear the bandwidth settings for unplugging and
shut down events? As far as my understanding, for both of these events,
the tap interface gets deleted, so there is no need to cleanup qdisc
settings of these devices.
It doesn't, actually. ... makes use of the device tap123 that already exists
and leaves it in the system. When I start and stop the domain with
the bandwidth set, it is kept in there. If I were to start another
domain without any bandwidth setting (or just use the device somehow),
the speed would be affected.
Probably taking Laine's approach would be cleaner and easier (it could
also take care of this automagically).
Looking forward to v2,
I am working on it right now and will send it out soon. Thanks for the
feedback.
Anirban
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] storage: zfs: implement pool build and delete
Provide an implementation for buildPool and deletePool operations for
the ZFS storage backend.
---
docs/schemas/storagepool.rng | 1 +
src/storage/storage_backend_zfs.c | 57 +++
2 files changed, 58 insertions(+)
diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng
index 908cc11..da8e1f3 100644
--- a/docs/schemas/storagepool.rng
+++ b/docs/schemas/storagepool.rng
@@ -386,6 +386,7 @@
+
diff --git a/src/storage/storage_backend_zfs.c
b/src/storage/storage_backend_zfs.c
index d8201ac..9482706 100644
--- a/src/storage/storage_backend_zfs.c
+++ b/src/storage/storage_backend_zfs.c
@@ -325,6 +325,61 @@ virStorageBackendZFSDeleteVol(virConnectPtr conn
ATTRIBUTE_UNUSED,
return ret;
}
+static int
+virStorageBackendZFSBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED,
+ virStoragePoolObjPtr pool,
+ unsigned int flags)
+{
+virCommandPtr cmd = NULL;
+size_t i;
+int ret = -1;
+
+virCheckFlags(0, -1);
+
+if (pool->def->source.ndevice == 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s", _("missing source devices"));
+return -1;
+}
+
+cmd = virCommandNewArgList(ZPOOL, "create",
+ pool->def->source.name, NULL);
+
+for (i = 0; i < pool->def->source.ndevice; i++)
+virCommandAddArg(cmd, pool->def->source.devices[i].path);
+
+if (virCommandRun(cmd, NULL) < 0)
+goto cleanup;
+
+ret = 0;
+
+ cleanup:
+virCommandFree(cmd);
+return ret;
+}
+
+static int
+virStorageBackendZFSDeletePool(virConnectPtr conn ATTRIBUTE_UNUSED,
+ virStoragePoolObjPtr pool,
+ unsigned int flags)
+{
+virCommandPtr cmd = NULL;
+int ret = -1;
+
+virCheckFlags(0, -1);
+
+cmd = virCommandNewArgList(ZPOOL, "destroy",
+ pool->def->source.name, NULL);
+
+if (virCommandRun(cmd, NULL) < 0)
+goto cleanup;
+
+ret = 0;
+
+ cleanup:
+virCommandFree(cmd);
+return ret;
+}
virStorageBackend virStorageBackendZFS = {
.type = VIR_STORAGE_POOL_ZFS,
@@ -333,6 +388,8 @@ virStorageBackend virStorageBackendZFS = {
.refreshPool = virStorageBackendZFSRefreshPool,
.createVol = virStorageBackendZFSCreateVol,
.deleteVol = virStorageBackendZFSDeleteVol,
+.buildPool = virStorageBackendZFSBuildPool,
+.deletePool = virStorageBackendZFSDeletePool,
.uploadVol = virStorageBackendVolUploadLocal,
.downloadVol = virStorageBackendVolDownloadLocal,
};
--
2.0.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] DHCP and secure containers
On 09/03/2014 09:42 AM, Gene Czarcinski wrote: On 09/02/2014 06:37 AM, Gene Czarcinski wrote: OK, hopefully this mailing list is more active and I can get some response to my questions. - I have been "playing with" Secure Containers running a lighttpd server and have it up and running. I used Adam's process (https://www.happyassassin.net/2014/07/23/bridged-networking-for-libvirt-with-networkmanager-2014-fedora-21/) for getting a bridge defined when also running NetworkManager. I then created a virtual network definition: net18 8d19a05b-ac85-4e2a-88bc-5ca4cbb29a33 This works fine when I use static addresses such as: -N source=net18,address=192.168.18.94/24,route=192.168.18.255%192.168.18.1 but does not work when I specify using dhcp: -N source=net18,dhcp I have reported this as a bug: https://bugzilla.redhat.com/show_bug.cgi?id=1133686 Since there has not been much of a reaction to the BZ report, I decided to take a look at the source code (it sure would have been nice if the SRPMS were there in the F20 fedora-virt-preview but I get the package from development/21). I see that libvirt-sandbox-init-common.c has the code for starting dhcp and also has main() along with some runtime options for -v verbose and -d debug. OK, how do I go about turning verbose and/or debug on? Any suggestions on how to debug and get dhcp to work? I not only want to find the problem but to fix the problem if needed. While I have not figured out how to get dhcp to work with a secure container create by virt-sandbox-service, I have gotten a container working with the network up and a dhcp assigned IP using the lxc-* commands and following this procedure: https://sysadmincasts.com/episodes/24-introduction-to-containers-on-linux-using-lxc to create a "busybox" container. The network came up automatically. Following the procedure in this tutorial: https://major.io/2014/04/21/launch-secure-lxc-containers-on-fedora-20-using-selinux-and-svirt/ I created and installed a test container. I had to add ifcfg-eth0 for a simple network and then run "service network start" for the netowrk to actually come up ... which it did with a DHCP (actually dnsmasq) assigned IP address. Note that this procedure explicitly installs the dhclient package. So, what am I doing "wrong" with secure containers? Or, is this a bug? Ping!! Hello ... anybody out there?? To keep my sanity, would SOMEBODY PLEASE try doing a secure sandbox with a dhcp network and see if the network is started or not. My case: static network started, dhcpnetwork is NOT started (/sbin/dhclient is not running). Here is what I have done so far: 1. "Instrumented" libvirt-sandbox-init-common.c and libvirt-sandbox-init-lxc.c by turning on debug and adding a whole bunch of fprintf(stderr,...) statements to track the initialization. These say that start_dhcp() in libvirt-sandbox-init-common.c is executed successfully. Nevertheless, for some reason, the g_spawn_async() did not result in a running /sbin/dhclient. 2. So, I tried running dhclient myself. I had two networks defined: "-N ,source=net18 -N dhcp,source=default". After connecting top the secure container, I did: /sbin/dhclient --no-pid eth1 which resulted in the network on eth1 starting with a 192.168.122. address. 3. I then went a step further. I took the start_dhcp() code from libvirt-sandbox-init-common.c and encapsulated it with a wrapper to fake what was done in init-common.c but with its own main(). Compiled this and put the binary where I could execute it after doing the connect. Stop, start, and connect to the secure container. The network on eth1 is not started. Run my test_dhcp_start program and the result was the eth1 network is started and there is a dhclient running. Suggestions please! Gene -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] ANNOUNCE: virt-manager 1.1.0 released
I'm happy to announce the release of virt-manager 1.1.0! virt-manager is a desktop application for managing KVM, Xen, and LXC virtualization via libvirt. The release can be downloaded from: http://virt-manager.org/download/ The direct download links are: http://virt-manager.org/download/sources/virt-manager/virt-manager-1.1.0.tar.gz This release includes: - Switch to libosinfo as OS metadata database (Giuseppe Scrivano) - Use libosinfo for OS detection from CDROM media labels (Giuseppe Scrivano) - Use libosinfo for improved OS defaults, like recommended disk size (Giuseppe Scrivano) - virt-image tool has been removed, as previously announced - Enable Hyper-V enlightenments for Windows VMs - Revert virtio-console default, back to plain serial console - Experimental q35 option in new VM 'customize' dialog - UI for virtual network QoS settings (Giuseppe Scrivano) - virt-install: --disk discard= support (Jim Minter) - addhardware: Add spiceport UI (Marc-André Lureau) - virt-install: --events on_poweroff etc. support (Chen Hanxiao) - cli --network portgroup= support and UI support - cli --boot initargs= and UI support - addhardware: allow setting controller model (Chen Hanxiao) - virt-install: support setting hugepage options (Chen Hanxiao) Thanks to everyone who has contributed to this release through testing, bug reporting, submitting patches, and otherwise sending in feedback! Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] DHCP and secure containers
On 09/07/2014 11:03 AM, Gene Czarcinski wrote: On 09/03/2014 09:42 AM, Gene Czarcinski wrote: On 09/02/2014 06:37 AM, Gene Czarcinski wrote: OK, hopefully this mailing list is more active and I can get some response to my questions. - I have been "playing with" Secure Containers running a lighttpd server and have it up and running. I used Adam's process (https://www.happyassassin.net/2014/07/23/bridged-networking-for-libvirt-with-networkmanager-2014-fedora-21/) for getting a bridge defined when also running NetworkManager. I then created a virtual network definition: net18 8d19a05b-ac85-4e2a-88bc-5ca4cbb29a33 This works fine when I use static addresses such as: -N source=net18,address=192.168.18.94/24,route=192.168.18.255%192.168.18.1 but does not work when I specify using dhcp: -N source=net18,dhcp I have reported this as a bug: https://bugzilla.redhat.com/show_bug.cgi?id=1133686 Since there has not been much of a reaction to the BZ report, I decided to take a look at the source code (it sure would have been nice if the SRPMS were there in the F20 fedora-virt-preview but I get the package from development/21). I see that libvirt-sandbox-init-common.c has the code for starting dhcp and also has main() along with some runtime options for -v verbose and -d debug. OK, how do I go about turning verbose and/or debug on? Any suggestions on how to debug and get dhcp to work? I not only want to find the problem but to fix the problem if needed. While I have not figured out how to get dhcp to work with a secure container create by virt-sandbox-service, I have gotten a container working with the network up and a dhcp assigned IP using the lxc-* commands and following this procedure: https://sysadmincasts.com/episodes/24-introduction-to-containers-on-linux-using-lxc to create a "busybox" container. The network came up automatically. Following the procedure in this tutorial: https://major.io/2014/04/21/launch-secure-lxc-containers-on-fedora-20-using-selinux-and-svirt/ I created and installed a test container. I had to add ifcfg-eth0 for a simple network and then run "service network start" for the netowrk to actually come up ... which it did with a DHCP (actually dnsmasq) assigned IP address. Note that this procedure explicitly installs the dhclient package. So, what am I doing "wrong" with secure containers? Or, is this a bug? Ping!! Hello ... anybody out there?? To keep my sanity, would SOMEBODY PLEASE try doing a secure sandbox with a dhcp network and see if the network is started or not. My case: static network started, dhcpnetwork is NOT started (/sbin/dhclient is not running). Here is what I have done so far: 1. "Instrumented" libvirt-sandbox-init-common.c and libvirt-sandbox-init-lxc.c by turning on debug and adding a whole bunch of fprintf(stderr,...) statements to track the initialization. These say that start_dhcp() in libvirt-sandbox-init-common.c is executed successfully. Nevertheless, for some reason, the g_spawn_async() did not result in a running /sbin/dhclient. 2. So, I tried running dhclient myself. I had two networks defined: "-N ,source=net18 -N dhcp,source=default". After connecting top the secure container, I did: /sbin/dhclient --no-pid eth1 which resulted in the network on eth1 starting with a 192.168.122. address. 3. I then went a step further. I took the start_dhcp() code from libvirt-sandbox-init-common.c and encapsulated it with a wrapper to fake what was done in init-common.c but with its own main(). Compiled this and put the binary where I could execute it after doing the connect. Stop, start, and connect to the secure container. The network on eth1 is not started. Run my test_dhcp_start program and the result was the eth1 network is started and there is a dhclient running. Suggestions please! This is getting really strange! I put a bash-shell-script wrapper around dhclient so that I could add a little logging when dhclient started. It is never executed!!! And yet, once the secure container has started, I can connect and manually run dhclient with no problems both direct command line and via a small fake-it program which runs g_spawn_async(). Part of the problem is that /usr/libexec/libvirt-sandbox-init-lxc and /usr/libexec/libvirt-sandbox-init-common run in the secure container environment but are also part of the software which initializes the secure container. At this point, I really wish that networking was a separate systemd service which was controlled by systemd. I wonder if there is some way to run gdb to help trace the execution. Next step ... convert to using g_spawn_sync() rather than g_spawn_async() to see if that produces any change. The g_spawn_sync() seems to work OK running "ip" to set up the static IP NIC. Gene -- libvir-lis
Re: [libvirt] [PATCH-RFC] qemu: Add network bandwidth setting for ethernet interfaces
On 9/7/14, 2:01 AM, "Martin Kletzander" wrote: >On Fri, Sep 05, 2014 at 11:42:58PM +, Anirban Chakraborty wrote: >> >> >>On 9/5/14, 1:31 AM, "Martin Kletzander" wrote: >> >>>On Thu, Sep 04, 2014 at 03:02:54PM -0700, Anirban Chakraborty wrote: >It doesn't, actually. dev="tap123"/> ... makes use of the device tap123 that already exists >and leaves it in the system. When I start and stop the domain with >the bandwidth set, it is kept in there. If I were to start another >domain without any bandwidth setting (or just use the device somehow), >the speed would be affected. > >Probably taking Laine's approach would be cleaner and easier (it could >also take care of this automagically). You are right that tap device is not deleted (as libvirt doesn¹t create it either) for ethernet type interfaces. However, for all other interface types, corresponding tap device is deleted. So, if I clear the bandwidth setting in a generic way it will become redundant for these other interface types. In any case, I¹ll ensure that the tap device configurations are cleared out in the unplug and shutdown path. Anirban -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH-RFC] qemu: Add network bandwidth setting for ethernet interfaces
On 9/5/14, 8:52 AM, "Laine Stump" wrote:
>On 09/05/2014 04:31 AM, Martin Kletzander wrote:
>> On Thu, Sep 04, 2014 at 03:02:54PM -0700, Anirban Chakraborty wrote:
>>> ethernet interfaces in libvirt currently do not support bandwidth
>>> setting.
>>> For example, following xml file for an interface will not apply these
>>> settings to corresponding qdiscs.
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> -
>>>
>>> This patch fixes the behavior.
>>
>> Although this doesn't confuse git, it might confuse something else.
>> Leaving it without '' is ok.
>>
>>> Please review it and if it appears ok, please apply.
>>>
>>> thanks,
>>> Anirban Chakraborty
>>>
>>
>> No need to have this in the commit message, it's kept in the log then.
>>
>>>
>>> Signed-off-by: Anirban Chakraborty
>>> ---
>>> src/qemu/qemu_command.c | 5 +
>>> src/qemu/qemu_hotplug.c | 3 +++
>>> 2 files changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index 2184caa..258c6a7 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -7251,6 +7251,11 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>>> if (tapfd[0] < 0)
>>> goto cleanup;
>>> }
>>> +/* Configure network bandwidth for ethernet type network
>>> interfaces */
>>> +if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)
>>> +if (virNetDevBandwidthSet(net->ifname,
>>> +virDomainNetGetActualBandwidth(net), false) < 0)
>>> +goto cleanup;
>>>
>>
>> In libvirt, we indent by spaces. This would be caught by running
>> 'make syntax-check'. Anyway, running 'make syntax-check check' is a
>> good practice to follow before formatting the patches.
>>
>>> if ((actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
>>> actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
>>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>>> index a364c52..aeb53c5 100644
>>> --- a/src/qemu/qemu_hotplug.c
>>> +++ b/src/qemu/qemu_hotplug.c
>>> @@ -940,6 +940,9 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>>> if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd,
>>> &vhostfdSize) < 0)
>>> goto cleanup;
>>> } else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) {
>>> +if (virNetDevBandwidthSet(net->ifname,
>>> +virDomainNetGetActualBandwidth(net), false) < 0)
>>> +goto cleanup;
>>
>> Same here.
>>
>> We need to clear the bandwidth settings when shutting down the domain
>> or unplugging the device.
>
>Aside from that, I would prefer if we could consolidate to *fewer* calls
>to virNetDevBandwidthSet() rather than adding more special case warts on
>the code. If the current calls to that function could be moved up in the
>call stack from their current low-level locations that are specific to a
>particular type of interface, perhaps they could all converge to a
>single place. Then, that single place could make the call for all
>interface types that supported bandwidth setting, and log an error
>message for all interface types that didn't support it.
Thanks for your suggestions. As a first step, I am creating a patch to
call virNetDevBandwidthSet() and Clear() from a higher level.
>
>(as a matter of fact, I would eventually like to get all stuff like this
>moved into the equivalent location to networkAllocateActualDevice(), so
>that 1) a single call would take care of *everything* for a network
>device, regardless of type, and 2) it would be possible to place that
>single function behind a public API that could be callable from
>session-mode libvirtd to enable unprivileged guests to have access to
>all the different types of network connectivity.)
This sounds great. However, this would require a lot more code changes. I
guess it could be done at a later point in time.
-Anirban
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/5] Fix problems caused by FD passing to session daemon
There were various problems introduced by the series on FD passing. The path for the socket was not created, the socket was not removed before binding it, etc. Let's see if it's any better this time. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1138604 Martin Kletzander (5): rpc: reformat the flow to make a bit more sense remove redundant pidfile path constructions util: fix potential leak in error codepath util: get rid of unnecessary umask() call rpc: make daemon spawning a bit more intelligent daemon/libvirtd.c | 41 ++ src/libvirt_private.syms | 1 + src/locking/lock_daemon.c | 42 ++- src/rpc/virnetsocket.c| 133 +++--- src/util/virpidfile.c | 48 - src/util/virpidfile.h | 7 ++- 6 files changed, 151 insertions(+), 121 deletions(-) -- 2.1.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] rpc: reformat the flow to make a bit more sense
Signed-off-by: Martin Kletzander
---
src/rpc/virnetsocket.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index 586a0d7..42184bd 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -574,13 +574,14 @@ int virNetSocketNewConnectUNIX(const char *path,
retry:
if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) {
+int status = 0;
+pid_t pid = 0;
+
if (!spawnDaemon) {
virReportSystemError(errno, _("Failed to connect socket to '%s'"),
path);
goto error;
-} else {
-int status = 0;
-pid_t pid = 0;
+}
if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) {
virReportSystemError(errno, "%s", _("Failed to create socket"));
@@ -634,7 +635,6 @@ int virNetSocketNewConnectUNIX(const char *path,
if (virNetSocketForkDaemon(binary, passfd) < 0)
goto error;
}
-}
localAddr.len = sizeof(localAddr.data);
if (getsockname(fd, &localAddr.data.sa, &localAddr.len) < 0) {
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/5] remove redundant pidfile path constructions
Signed-off-by: Martin Kletzander
---
daemon/libvirtd.c | 41 ---
src/libvirt_private.syms | 1 +
src/locking/lock_daemon.c | 42
src/util/virpidfile.c | 49 ++-
src/util/virpidfile.h | 7 ++-
5 files changed, 63 insertions(+), 77 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 0503cd0..61f5486 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -251,41 +251,6 @@ static int daemonForkIntoBackground(const char *argv0)
static int
-daemonPidFilePath(bool privileged,
- char **pidfile)
-{
-if (privileged) {
-if (VIR_STRDUP(*pidfile, LOCALSTATEDIR "/run/libvirtd.pid") < 0)
-goto error;
-} else {
-char *rundir = NULL;
-mode_t old_umask;
-
-if (!(rundir = virGetUserRuntimeDirectory()))
-goto error;
-
-old_umask = umask(077);
-if (virFileMakePath(rundir) < 0) {
-umask(old_umask);
-goto error;
-}
-umask(old_umask);
-
-if (virAsprintf(pidfile, "%s/libvirtd.pid", rundir) < 0) {
-VIR_FREE(rundir);
-goto error;
-}
-
-VIR_FREE(rundir);
-}
-
-return 0;
-
- error:
-return -1;
-}
-
-static int
daemonUnixSocketPaths(struct daemonConfig *config,
bool privileged,
char **sockfile,
@@ -1313,8 +1278,10 @@ int main(int argc, char **argv) {
}
if (!pid_file &&
-daemonPidFilePath(privileged,
- &pid_file) < 0) {
+virPidFileConstructPath(privileged,
+LOCALSTATEDIR,
+"libvirtd",
+&pid_file) < 0) {
VIR_ERROR(_("Can't determine pid file path."));
exit(EXIT_FAILURE);
}
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index f136d24..9c6ab77 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1774,6 +1774,7 @@ virPCIIsVirtualFunction;
virPidFileAcquire;
virPidFileAcquirePath;
virPidFileBuildPath;
+virPidFileConstructPath;
virPidFileDelete;
virPidFileDeletePath;
virPidFileRead;
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index 02d77e3..fe7cfb8 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -366,42 +366,6 @@ virLockDaemonForkIntoBackground(const char *argv0)
static int
-virLockDaemonPidFilePath(bool privileged,
- char **pidfile)
-{
-if (privileged) {
-if (VIR_STRDUP(*pidfile, LOCALSTATEDIR "/run/virtlockd.pid") < 0)
-goto error;
-} else {
-char *rundir = NULL;
-mode_t old_umask;
-
-if (!(rundir = virGetUserRuntimeDirectory()))
-goto error;
-
-old_umask = umask(077);
-if (virFileMakePath(rundir) < 0) {
-umask(old_umask);
-goto error;
-}
-umask(old_umask);
-
-if (virAsprintf(pidfile, "%s/virtlockd.pid", rundir) < 0) {
-VIR_FREE(rundir);
-goto error;
-}
-
-VIR_FREE(rundir);
-}
-
-return 0;
-
- error:
-return -1;
-}
-
-
-static int
virLockDaemonUnixSocketPaths(bool privileged,
char **sockfile)
{
@@ -1283,8 +1247,10 @@ int main(int argc, char **argv) {
}
if (!pid_file &&
-virLockDaemonPidFilePath(privileged,
- &pid_file) < 0) {
+virPidFileConstructPath(privileged,
+LOCALSTATEDIR,
+"virtlockd",
+&pid_file) < 0) {
VIR_ERROR(_("Can't determine pid file path."));
exit(EXIT_FAILURE);
}
diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c
index 1d9a1c5..19ec103 100644
--- a/src/util/virpidfile.c
+++ b/src/util/virpidfile.c
@@ -1,7 +1,7 @@
/*
* virpidfile.c: manipulation of pidfiles
*
- * Copyright (C) 2010-2012 Red Hat, Inc.
+ * Copyright (C) 2010-2012, 2014 Red Hat, Inc.
* Copyright (C) 2006, 2007 Binary Karma
* Copyright (C) 2006 Shuveb Hussain
*
@@ -521,3 +521,50 @@ int virPidFileRelease(const char *dir,
VIR_FREE(pidfile);
return rc;
}
+
+
+int
+virPidFileConstructPath(bool privileged,
+const char *statedir,
+const char *progname,
+char **pidfile)
+{
+if (privileged) {
+/*
+ * This is here just to allow calling this function with
+ * statedir == NULL; of course only when !privileged.
+ */
+if (!statedir) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("No statedir specified"));
+goto cleanup;
+}
+if (virAsprintf(pidfile, "%s/run/%s.pid", statedir, progname) < 0
[libvirt] [PATCH 3/5] util: fix potential leak in error codepath
Signed-off-by: Martin Kletzander
---
src/util/virpidfile.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c
index 19ec103..dd29701 100644
--- a/src/util/virpidfile.c
+++ b/src/util/virpidfile.c
@@ -529,6 +529,9 @@ virPidFileConstructPath(bool privileged,
const char *progname,
char **pidfile)
{
+int ret = -1;
+char *rundir = NULL;
+
if (privileged) {
/*
* This is here just to allow calling this function with
@@ -542,29 +545,27 @@ virPidFileConstructPath(bool privileged,
if (virAsprintf(pidfile, "%s/run/%s.pid", statedir, progname) < 0)
goto cleanup;
} else {
-char *rundir = NULL;
mode_t old_umask;
if (!(rundir = virGetUserRuntimeDirectory()))
-goto error;
+goto cleanup;
old_umask = umask(077);
if (virFileMakePath(rundir) < 0) {
umask(old_umask);
-goto error;
+goto cleanup;
}
umask(old_umask);
if (virAsprintf(pidfile, "%s/%s.pid", rundir, progname) < 0) {
VIR_FREE(rundir);
-goto error;
+goto cleanup;
}
-VIR_FREE(rundir);
}
-return 0;
-
- error:
-return -1;
+ret = 0;
+ cleanup:
+VIR_FREE(rundir);
+return ret;
}
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/5] util: get rid of unnecessary umask() call
Signed-off-by: Martin Kletzander
---
src/util/virpidfile.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c
index dd29701..a3b8846 100644
--- a/src/util/virpidfile.c
+++ b/src/util/virpidfile.c
@@ -545,17 +545,15 @@ virPidFileConstructPath(bool privileged,
if (virAsprintf(pidfile, "%s/run/%s.pid", statedir, progname) < 0)
goto cleanup;
} else {
-mode_t old_umask;
-
if (!(rundir = virGetUserRuntimeDirectory()))
goto cleanup;
-old_umask = umask(077);
-if (virFileMakePath(rundir) < 0) {
-umask(old_umask);
+if (virFileMakePathWithMode(rundir, 0700) < 0) {
+virReportSystemError(errno,
+ _("Cannot create user runtime directory
'%s'"),
+ rundir);
goto cleanup;
}
-umask(old_umask);
if (virAsprintf(pidfile, "%s/%s.pid", rundir, progname) < 0) {
VIR_FREE(rundir);
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] util: let virSetSockReuseAddr report unified error message
Signed-off-by: Martin Kletzander
---
src/rpc/virnetsocket.c | 7 ++-
src/util/virportallocator.c | 5 +
src/util/virutil.c | 13 ++---
src/util/virutil.h | 2 +-
4 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index 586a0d7..2192dc8 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -278,10 +278,8 @@ int virNetSocketNewListenTCP(const char *nodename,
goto error;
}
-if (virSetSockReuseAddr(fd) < 0) {
-virReportSystemError(errno, "%s", _("Unable to enable port
reuse"));
+if (virSetSockReuseAddr(fd, true) < 0)
goto error;
-}
#ifdef IPV6_V6ONLY
if (runp->ai_family == PF_INET6) {
@@ -493,9 +491,8 @@ int virNetSocketNewConnectTCP(const char *nodename,
goto error;
}
-if (virSetSockReuseAddr(fd) < 0) {
+if (virSetSockReuseAddr(fd, false) < 0)
VIR_WARN("Unable to enable port reuse");
-}
if (connect(fd, runp->ai_addr, runp->ai_addrlen) >= 0)
break;
diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c
index ff5691a..f1dade3 100644
--- a/src/util/virportallocator.c
+++ b/src/util/virportallocator.c
@@ -142,11 +142,8 @@ static int virPortAllocatorBindToPort(bool *used,
goto cleanup;
}
-if (virSetSockReuseAddr(fd) < 0) {
-virReportSystemError(errno, "%s",
- _("Unable to set socket reuse addr flag"));
+if (virSetSockReuseAddr(fd, true) < 0)
goto cleanup;
-}
if (ipv6 && setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, (void*)&v6only,
sizeof(v6only)) < 0) {
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 04113bb..8d2f62a 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -148,7 +148,7 @@ int virSetCloseExec(int fd)
}
#ifdef WIN32
-int virSetSockReuseAddr(int fd ATTRIBUTE_UNUSED)
+int virSetSockReuseAddr(int fd ATTRIBUTE_UNUSED, bool fatal ATTRIBUTE_UNUSED)
{
/*
* SO_REUSEADDR on Windows is actually akin to SO_REUSEPORT
@@ -163,10 +163,17 @@ int virSetSockReuseAddr(int fd ATTRIBUTE_UNUSED)
return 0;
}
#else
-int virSetSockReuseAddr(int fd)
+int virSetSockReuseAddr(int fd, bool fatal)
{
int opt = 1;
-return setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt));
+int ret = setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt));
+
+if (ret < 0 && fatal) {
+virReportSystemError(errno, "%s",
+ _("Unable to set socket reuse addr flag"));
+}
+
+return ret;
}
#endif
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 89b7923..54f1148 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -43,7 +43,7 @@ int virSetBlocking(int fd, bool blocking)
ATTRIBUTE_RETURN_CHECK;
int virSetNonBlock(int fd) ATTRIBUTE_RETURN_CHECK;
int virSetInherit(int fd, bool inherit) ATTRIBUTE_RETURN_CHECK;
int virSetCloseExec(int fd) ATTRIBUTE_RETURN_CHECK;
-int virSetSockReuseAddr(int fd) ATTRIBUTE_RETURN_CHECK;
+int virSetSockReuseAddr(int fd, bool fatal) ATTRIBUTE_RETURN_CHECK;
int virPipeReadUntilEOF(int outfd, int errfd,
char **outbuf, char **errbuf);
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/5] rpc: make daemon spawning a bit more intelligent
This way it behaves more like the daemon itself does (acquiring a
pidfile, deleting the socket before binding, etc.).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1138604
Signed-off-by: Martin Kletzander
---
src/rpc/virnetsocket.c | 57 --
1 file changed, 51 insertions(+), 6 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index 42184bd..18a5a8f 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -51,9 +51,11 @@
#include "virlog.h"
#include "virfile.h"
#include "virthread.h"
+#include "virpidfile.h"
#include "virprobe.h"
#include "virprocess.h"
#include "virstring.h"
+#include "dirname.h"
#include "passfd.h"
#if WITH_SSH2
@@ -544,7 +546,10 @@ int virNetSocketNewConnectUNIX(const char *path,
const char *binary,
virNetSocketPtr *retsock)
{
+char *binname = NULL;
+char *pidpath = NULL;
int fd, passfd = -1;
+int pidfd = -1;
virSocketAddr localAddr;
virSocketAddr remoteAddr;
@@ -583,16 +588,45 @@ int virNetSocketNewConnectUNIX(const char *path,
goto error;
}
+if (!(binname = last_component(binary)) || binname[0] == '\0') {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot determine basename for binary '%s'"),
+ binary);
+goto error;
+}
+
+if (virPidFileConstructPath(false, NULL, binname, &pidpath) < 0)
+goto error;
+
+if ((pidfd = virPidFileAcquirePath(pidpath, false, getpid())) < 0) {
+/*
+ * This can happen in a very rare case of two clients spawning two
+ * daemons at the same time, and the error in the logs that gets
+ * reset here can be a clue to some future debugging.
+ */
+virResetLastError();
+spawnDaemon = false;
+goto retry;
+}
+
if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) {
virReportSystemError(errno, "%s", _("Failed to create socket"));
goto error;
}
/*
- * We have to fork() here, because umask() is set
- * per-process, chmod() is racy and fchmod() has undefined
- * behaviour on sockets according to POSIX, so it doesn't
- * work outside Linux.
+ * We already even acquired the pidfile, so noone else should be using
+ * @path right now. So we're OK to unlink it and paying attention to
+ * the return value makes no real sense here. Only if it's not an
+ * abstract socket, of course.
+ */
+if (path[0] != '@')
+unlink(path);
+
+/*
+ * We have to fork() here, because umask() is set per-process, chmod()
+ * is racy and fchmod() has undefined behaviour on sockets according to
+ * POSIX, so it doesn't work outside Linux.
*/
if ((pid = virFork()) < 0)
goto error;
@@ -612,8 +646,9 @@ int virNetSocketNewConnectUNIX(const char *path,
/*
* OK, so the subprocces failed to bind() the socket. This may
mean
* that another daemon was starting at the same time and succeeded
- * with its bind(). So we'll try connecting again, but this time
- * without spawning the daemon.
+ * with its bind() (even though it should not happen because we
+ * using a pidfile for the race). So we'll try connecting again,
+ * but this time without spawning the daemon.
*/
spawnDaemon = false;
goto retry;
@@ -632,6 +667,12 @@ int virNetSocketNewConnectUNIX(const char *path,
goto error;
}
+/*
+ * Do we need to eliminate the super-rare race here any more? It would
+ * need incorporating the following VIR_FORCE_CLOSE() into a
+ * virCommandHook inside a virNetSocketForkDaemon().
+ */
+VIR_FORCE_CLOSE(pidfd);
if (virNetSocketForkDaemon(binary, passfd) < 0)
goto error;
}
@@ -648,8 +689,12 @@ int virNetSocketNewConnectUNIX(const char *path,
return 0;
error:
+if (pidfd > 0)
+virPidFileDeletePath(pidpath);
+VIR_FREE(pidpath);
VIR_FORCE_CLOSE(fd);
VIR_FORCE_CLOSE(passfd);
+VIR_FORCE_CLOSE(pidfd);
if (spawnDaemon)
unlink(path);
return -1;
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
