Re: [libvirt PATCH v2 08/33] systemd: Switch virtnodedevd to common templates
On Thu, Sep 28, 2023 at 05:38:45AM -0500, Andrea Bolognani wrote: > On Thu, Sep 28, 2023 at 11:16:53AM +0100, Daniel P. Berrangé wrote: > > On Wed, Sep 27, 2023 at 06:19:09PM +0200, Andrea Bolognani wrote: > > > Signed-off-by: Andrea Bolognani > > > --- > > > src/node_device/meson.build | 4 > > > src/node_device/virtnodedevd.service.in | 25 - > > > > > 2 files changed, 29 deletions(-) > > > delete mode 100644 src/node_device/virtnodedevd.service.in > > > > Reviewed-by: Daniel P. Berrangé > > > > Though I wonder if its worth just keeping an empty stub here, with > > the section headings. It'd be a little confusing to see the stubs > > present for some daemons but not others. > > We'd have to do the same for sockets then, on account of virtxend > using an override for them. > > Maybe we could change the merge script so that contents before the > start of the first section are simply ignored, and then have > > $ cat src/node_device/virtnodedevd.service.in > # Merged into src/virtd.service.in > $ cat src/node_device/virtnodedevd.socket.in > # Merged into src/virtd*.socket.in > > for services that don't need any overrides, and > > $ cat src/libxl/virtxend.service.extra.in > # Merged into src/virtd.service.in > > [Unit] > Wants=virtlockd.socket > After=virtlockd.socket > ... > $ cat src/libxl/virtxend.socket.extra.in > # Merged into src/virtd*.socket.in > > [Unit] > ConditionPathExists=/proc/xen/capabilities > > for services that do. It would mean introducing quite a number of > additional files, but maybe the advantages in terms of > discoverability make up for that downside? Yeah, I think that's a nice idea. > If we allow empty overrides, we might be even able to simplify the > way the various services are defined in their meson.build files, by > somehow deriving the path of the file instead of requiring it to be > provided explicitly. That part could be tricky though. > > Overall I'm not opposed to the idea, but let's consider it for a > follow-up instead of stalling this further, okay? Sure With 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 :|
Re: [libvirt PATCH v2 08/33] systemd: Switch virtnodedevd to common templates
On Thu, Sep 28, 2023 at 11:16:53AM +0100, Daniel P. Berrangé wrote: > On Wed, Sep 27, 2023 at 06:19:09PM +0200, Andrea Bolognani wrote: > > Signed-off-by: Andrea Bolognani > > --- > > src/node_device/meson.build | 4 > > src/node_device/virtnodedevd.service.in | 25 - > > > 2 files changed, 29 deletions(-) > > delete mode 100644 src/node_device/virtnodedevd.service.in > > Reviewed-by: Daniel P. Berrangé > > Though I wonder if its worth just keeping an empty stub here, with > the section headings. It'd be a little confusing to see the stubs > present for some daemons but not others. We'd have to do the same for sockets then, on account of virtxend using an override for them. Maybe we could change the merge script so that contents before the start of the first section are simply ignored, and then have $ cat src/node_device/virtnodedevd.service.in # Merged into src/virtd.service.in $ cat src/node_device/virtnodedevd.socket.in # Merged into src/virtd*.socket.in for services that don't need any overrides, and $ cat src/libxl/virtxend.service.extra.in # Merged into src/virtd.service.in [Unit] Wants=virtlockd.socket After=virtlockd.socket ... $ cat src/libxl/virtxend.socket.extra.in # Merged into src/virtd*.socket.in [Unit] ConditionPathExists=/proc/xen/capabilities for services that do. It would mean introducing quite a number of additional files, but maybe the advantages in terms of discoverability make up for that downside? If we allow empty overrides, we might be even able to simplify the way the various services are defined in their meson.build files, by somehow deriving the path of the file instead of requiring it to be provided explicitly. That part could be tricky though. Overall I'm not opposed to the idea, but let's consider it for a follow-up instead of stalling this further, okay? -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH v2 08/33] systemd: Switch virtnodedevd to common templates
On Wed, Sep 27, 2023 at 06:19:09PM +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > src/node_device/meson.build | 4 > src/node_device/virtnodedevd.service.in | 25 - > 2 files changed, 29 deletions(-) > delete mode 100644 src/node_device/virtnodedevd.service.in Reviewed-by: Daniel P. Berrangé Though I wonder if its worth just keeping an empty stub here, with the section headings. It'd be a little confusing to see the stubs present for some daemons but not others. With 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 :|
[libvirt PATCH v2 08/33] systemd: Switch virtnodedevd to common templates
Signed-off-by: Andrea Bolognani
---
src/node_device/meson.build | 4
src/node_device/virtnodedevd.service.in | 25 -
2 files changed, 29 deletions(-)
delete mode 100644 src/node_device/virtnodedevd.service.in
diff --git a/src/node_device/meson.build b/src/node_device/meson.build
index dd60b1f819..2614ff8b9c 100644
--- a/src/node_device/meson.build
+++ b/src/node_device/meson.build
@@ -52,11 +52,7 @@ if conf.has('WITH_NODE_DEVICES')
virt_daemon_units += {
'service': 'virtnodedevd',
-'service_in': files('virtnodedevd.service.in'),
'name': 'Libvirt nodedev',
-'socket_in': libvirtd_socket_in,
-'socket_ro_in': libvirtd_socket_ro_in,
-'socket_admin_in': libvirtd_socket_admin_in,
}
openrc_init_files += {
diff --git a/src/node_device/virtnodedevd.service.in
b/src/node_device/virtnodedevd.service.in
deleted file mode 100644
index 2ac41db32e..00
--- a/src/node_device/virtnodedevd.service.in
+++ /dev/null
@@ -1,25 +0,0 @@
-[Unit]
-Description=Virtualization nodedev daemon
-Conflicts=libvirtd.service
-Requires=virtnodedevd.socket
-Requires=virtnodedevd-ro.socket
-Requires=virtnodedevd-admin.socket
-After=network.target
-After=dbus.service
-After=apparmor.service
-Documentation=man:virtnodedevd(8)
-Documentation=https://libvirt.org
-
-[Service]
-Type=notify
-Environment=VIRTNODEDEVD_ARGS="--timeout 120"
-EnvironmentFile=-@initconfdir@/virtnodedevd
-ExecStart=@sbindir@/virtnodedevd $VIRTNODEDEVD_ARGS
-ExecReload=/bin/kill -HUP $MAINPID
-Restart=on-failure
-
-[Install]
-WantedBy=multi-user.target
-Also=virtnodedevd.socket
-Also=virtnodedevd-ro.socket
-Also=virtnodedevd-admin.socket
--
2.41.0
