On Mon, Aug 31, 2020 at 09:45:15AM +0200, Katerina Koukiou wrote:
> Signed-off-by: Katerina Koukiou <kkoukiou@xxxxxxxxxx>
> ---
> data/system/libvirt-dbus.service.in | 21 +++++++++++++++
> data/system/meson.build | 30 +++++++++++++++++-----
> data/system/org.libvirt-systemd.service.in | 5 ++++
> libvirt-dbus.spec.in | 4 ++-
> meson.build | 12 +++++++++
> meson_options.txt | 1 +
> 6 files changed, 66 insertions(+), 7 deletions(-)
> create mode 100644 data/system/libvirt-dbus.service.in
> create mode 100644 data/system/org.libvirt-systemd.service.in
>
> diff --git data/system/libvirt-dbus.service.in data/system/libvirt-dbus.service.in
> new file mode 100644
> index 0000000..862a366
> --- /dev/null
> +++ data/system/libvirt-dbus.service.in
> @@ -0,0 +1,21 @@
> +# SPDX-License-Identifier: LGPL-2.1+
> +#
> +# This file is part of systemd.
> +#
> +# systemd is free software; you can redistribute it and/or modify it
> +# under the terms of the GNU Lesser General Public License as published by
> +# the Free Software Foundation; either version 2.1 of the License, or
> +# (at your option) any later version.
This doesn't look correct as you are contributing to libvirt-dbus, not
systemd.
> +
> +[Unit]
> +Description=Libvirt DBus Service
> +
> +[Service]
> +BusName=org.libvirt
> +DynamicUser=yes
> +User=@SYSTEM_USER@
> +Group=@SYSTEM_USER@
> +ExecStart=@sbindir@/libvirt-dbus --system
> +
> +[Install]
> +Alias=org.libvirt.service
> diff --git data/system/meson.build data/system/meson.build
> index 74f1949..67657d4 100644
> --- data/system/meson.build
> +++ data/system/meson.build
> @@ -1,9 +1,18 @@
> -configure_file(
> - configuration: conf,
> - input: 'org.libvirt.service.in',
> - output: 'org.libvirt.service',
> - install_dir: dbus_system_services_dir,
> -)
> +if init_script == 'systemd'
> + configure_file(
> + configuration: conf,
> + input: 'org.libvirt-systemd.service.in',
> + output: 'org.libvirt.service',
> + install_dir: dbus_system_services_dir,
> + )
> +else
> + configure_file(
> + configuration: conf,
> + input: 'org.libvirt.service.in',
> + output: 'org.libvirt.service',
> + install_dir: dbus_system_services_dir,
> + )
> +endif
Here you can use this to reduce code duplication:
if init_script == 'systemd'
dbus_service_in = 'org.libvirt-systemd.service.in'
else
dbus_service_in = 'org.libvirt.service.in'
endif
configure_file(
configuration: conf,
input: dbus_service_in,
output: 'org.libvirt.service',
install_dir: dbus_system_services_dir,
)
> configure_file(
> configuration: conf,
> input: 'org.libvirt.conf.in',
> @@ -16,3 +25,12 @@ configure_file(
> output: 'libvirt-dbus.rules',
> install_dir: polkit_rules_dir,
> )
> +if init_script == 'systemd'
> + systemd_system_unit_dir = systemd_dep.get_pkgconfig_variable('systemdsystemunitdir')
> + configure_file(
> + configuration: conf,
> + input: 'libvirt-dbus.service.in',
> + output: 'libvirt-dbus.service',
> + install_dir: systemd_system_unit_dir,
> + )
> +endif
> diff --git data/system/org.libvirt-systemd.service.in data/system/org.libvirt-systemd.service.in
> new file mode 100644
> index 0000000..ba260b2
> --- /dev/null
> +++ data/system/org.libvirt-systemd.service.in
> @@ -0,0 +1,5 @@
> +[D-BUS Service]
> +Name=org.libvirt
> +Exec=/bin/false
> +User=@SYSTEM_USER@
> +SystemdService=libvirt-dbus.service
> diff --git libvirt-dbus.spec.in libvirt-dbus.spec.in
> index 4e6ff85..8286609 100644
> --- libvirt-dbus.spec.in
> +++ libvirt-dbus.spec.in
> @@ -40,7 +40,8 @@ This package provides D-Bus API for libvirt
> %autosetup
>
> %build
> -%meson
> +%meson \
> + -Dinit_script=systemd
I would say 4 spaces are good enough as indentation.
> %meson_build
>
> %install
> @@ -57,6 +58,7 @@ exit 0
> %doc AUTHORS.rst NEWS.rst
> %license COPYING
> %{_sbindir}/libvirt-dbus
> +%{_unitdir}/libvirt-dbus.service
> %{_datadir}/dbus-1/services/org.libvirt.service
> %{_datadir}/dbus-1/system-services/org.libvirt.service
> %{_datadir}/dbus-1/system.d/org.libvirt.conf
> diff --git meson.build meson.build
> index e765ed6..c34c07d 100644
> --- meson.build
> +++ meson.build
> @@ -12,6 +12,18 @@ project(
> prefix = get_option('prefix')
> datadir = prefix / get_option('datadir')
> sbindir = prefix / get_option('sbindir')
> +if get_option('init_script') == 'check'
> + if find_program('systemctl', required: false).found()
> + init_script = 'systemd'
> + else
> + init_script = 'other'
> + endif
> +else
> + init_script = get_option('init_script')
> +endif
Inconsistent indentation, in libvirt-dbus we use 4 spaces.
> +if init_script == 'systemd'
> + systemd_dep = dependency('systemd')
> +endif
>
> opt_dirs = [
> 'dbus_interfaces',
> diff --git meson_options.txt meson_options.txt
> index 36e8065..41d348f 100644
> --- meson_options.txt
> +++ meson_options.txt
> @@ -4,3 +4,4 @@ option('dbus_system_policies', type: 'string', value: 'dbus-1/system.d', descrip
> option('dbus_interfaces', type: 'string', value: 'dbus-1/interfaces', description: 'D-Bus interfaces directory')
> option('polkit_rules', type: 'string', value: 'polkit-1/rules.d', description: 'polkit rules directory')
> option('system_user', type: 'string', value: 'libvirtdbus', description: 'username to run system instance as')
> +option('init_script', type: 'combo', choices: ['systemd', 'other', 'check'], value: 'check', description: 'Style of init script to install')
> --
> 2.26.2
>
Overall looks good.
One note though, libvirt-dbus switched to gitlab and uses merge request
workflow. So would you please create a merge request here:
https://gitlab.com/libvirt/libvirt-dbus
Pavel
Attachment:
signature.asc
Description: PGP signature
