Alon Bar-Lev has posted comments on this change.
Change subject: Split vdsm init script into pieces
......................................................................
Patch Set 3: (1 inline comment)
First I would like to thank you for the effort!
However, before I continue th review...
All I meant was to split the service into common generic part and downstream
specific part.
You have done much more, which is good but have its limits.
Comment for your specific implementation first:
1. It is not required to have all scripts as template. Use one template and
source all variables. The location of the template can be provided via the
environment from the root script.
2. env_common.sh.in - should not include any downstream specific code, in this
case /etc/init.d/functions, the use of downstream specific code should be only
at root script.
3. scripts should not be aware of systemd/initd directly but have options to
enable/disable specific functionality.
4. we should provide a different usage notation and wrapper script, see my
comment from May 17, we need the same syntax more or less and same wrappers
more or less.
5. libvirt reconfigure should probably moved to vdsm-tool, I already send a
message to danken about that regardless of this solution.
The limitation is that we cannot create dependency between on phase and another
phase. The hook usage is nice but will probably create extra complexity in
future.
If we write this in shell, I expect that all hooks are sourced into single
shell, and then executed within the same environment.
For syntax, while we at it...
1. every shell variable should be written as "${xxx}", mind the quotes
especially...
2. please use $(..) instead of `...`
I also think we should wait for[1] to settle before continuing, no?
Thanks!
[1] http://gerrit.ovirt.org/#/c/11051/
....................................................
File configure.ac
Line 206:
Line 207: AC_CHECK_PATH([LIBVIRT_DEFAULT],
Line 208: [/etc/sysconfig/libvirtd \
Line 209: /etc/default/libvirt-bin],
Line 210: [/etc/sysconfig/libvirtd])
Please don't use AC_ prefix, this is reserved for autoconf macros... use your
own.
Please allow overriding these from environment so that packaging code can
override it... Should be:
./configure --with-lock-dir=/var/lock
--with-libvirt-config=/etc/default/libvirt-bin
Then, the logic is within packaging not upstream code.
Line 211:
Line 212: # Keep sorted
Line 213: AC_OUTPUT([
Line 214: Makefile
--
To view, visit http://gerrit.ovirt.org/14826
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0c652704000c764c6e6f248605c6a3f4f3af5ace
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Zhou Zheng Sheng <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Mark Wu <[email protected]>
Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
