[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Add support for systemd init service



On 11/11/2011 07:32 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
> 
> This patch adds support for a systemd init service for libvirtd
> and libvirt-guests. The libvirtd.service is *not* written to use
> socket activation, since we want libvirtd to start on boot so it
> can do guest auto-start.
> 

> +++ b/configure.ac
> @@ -329,16 +329,30 @@ dnl init script flavor
>  dnl
>  AC_MSG_CHECKING([for init script flavor])
>  AC_ARG_WITH([init-script],
> -            [AC_HELP_STRING([--with-init-script=@<:@redhat|auto|none@:>@],
> +            [AC_HELP_STRING([--with-init-script=@<:@redhat|systemd|systemd+redhat|upstart|auto|none@:>@],
>  		     [Style of init script to install @<:@default=auto@:>@])])

That's a bit long.  Perhaps it would be better as:

AC_HELP_STRING([--with-init-script@<:@=STYLE@:>@],
  [Style of init script to install: redhat, systemd, systemd+redhat,
upstart, auto, none @<:@default=auto@:>@])

so that ./configure --help can take advantage of better word wrap

> -AM_CONDITIONAL([LIBVIRT_INIT_SCRIPT_RED_HAT], test x$with_init_script = xredhat)
> +init_redhat=no
> +init_systemd=no
> +case "$with_init_script" in
> +    systemd+redhat)
> +       init_redhat=yes
> +       init_systemd=yes
> +       ;;
> +    systemd)
> +       init_systemd=yes
> +       ;;
> +    redhat)
> +       init_redhat=yes
> +       ;;
> +    *)
> +       if test "$cross_compiling" != yes && test -f /etc/redhat-release; then
> +          init_redhat=yes
> +          with_init_script=redhat

Shouldn't this line be 'init_redhat=yes'?

> @@ -111,6 +112,11 @@
>  %define with_hyperv 0
>  %endif
>  
> +# Although earlier Fedora has systemd, libvirt still used sysvinit
> +%if 0%{?fedora} >= 17
> +%define with_systemd 1
> +%endif

But if we use the configure option, then what's to stop the systemd
script from working in older Fedora?  That is, why not make this >= 16,
not 17, so that people using the virt-preview repo to get 0.9.8 on F16
will benefit from systemd?

> +++ b/po/POTFILES.in
> @@ -151,5 +151,5 @@ src/xenapi/xenapi_utils.c
>  src/xenxs/xen_sxpr.c
>  src/xenxs/xen_xm.c
>  tools/console.c
> -tools/libvirt-guests.init.sh
> +tools/libvirt-guests.init.in
...
> diff --git a/tools/libvirt-guests.init.sh b/tools/libvirt-guests.init.in
> similarity index 100%
> rename from tools/libvirt-guests.init.sh
> rename to tools/libvirt-guests.init.in

I'm a bit worried on whether this will do the correct things with
translations embedded in the file.  My recollection was that we _had_ to
name it .sh instead of .in in order to get xgettext to properly parse
out the strings marked for translations into the .pot file.  I haven't
yet double-checked the resulting .pot file pre- and post-patch, but
think you may have to revert this particular change.

Overall, though, I think the patch is good.

-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]