Dan Kenigsberg has posted comments on this change. Change subject: BZ#734847- vdsmd always restarts libvirtd upon start-up ......................................................................
Patch Set 13: I would prefer that you didn't submit this (6 inline comments) .................................................... Commit Message Line 3: AuthorDate: 2012-08-29 19:22:03 +0300 Line 4: Commit: Yaniv Bronhaim <ybron...@redhat.com> Line 5: CommitDate: 2012-08-29 19:22:03 +0300 Line 6: Line 7: BZ#734847- vdsmd always restarts libvirtd upon start-up I hate to play policeman and start enforcing this, but please use Bug-Id: https://bugzilla.redhat.com/734847 tag. See the boring thread ending in http://lists.ovirt.org/pipermail/arch/2012-August/000823.html Line 8: Line 9: When starting vdsm we need to verify that libvirt is running by Line 10: upstart and not by SysV. Line 11: .................................................... File vdsm/vdsmd.init.in Line 146: done Line 147: return 0 Line 148: } Line 149: Line 150: is_sysv_exist() { you probably mean upstart_exists() but I think that a better name is libvirt_should_use_upstart() which we could in the future compute more semsibly. Line 151: [[ -x /sbin/initctl ]] Line 152: } Line 153: Line 154: start_needed_srv() { Line 439: return 1 Line 440: } Line 441: Line 442: is_already_run_with_upstart(){ Line 443: packaged=`/bin/rpm -ql libvirt libvirt-daemon | \ I do not quite like the code duplication here Line 444: /bin/grep libvirtd.upstart | /usr/bin/tail -1` Line 445: target=/etc/init/libvirtd.conf Line 446: if diff -q "$packaged" "$target" >/dev/null 2>&1; Line 447: then Line 441: Line 442: is_already_run_with_upstart(){ Line 443: packaged=`/bin/rpm -ql libvirt libvirt-daemon | \ Line 444: /bin/grep libvirtd.upstart | /usr/bin/tail -1` Line 445: target=/etc/init/libvirtd.conf wouldn't a simple [[ -f /etc/init/libvirtd.conf ]] be enough for your needs? Line 446: if diff -q "$packaged" "$target" >/dev/null 2>&1; Line 447: then Line 448: return 0 Line 449: fi Line 450: return 1 Line 451: } Line 452: Line 453: is_libvirtd_run(){ Line 454: if pgrep libvirtd > /dev/null 2>&1; dropping the whole "if" gives the same semantics. Line 455: then Line 456: return 0 Line 457: fi Line 458: return 1 Line 485: then Line 486: return Line 487: fi Line 488: Line 489: startout=`/sbin/initctl start libvirtd 2>&1` could you explain (in the commit message) where a running libvirt is killed and restarted? is it here? or line 470? or both? Line 490: if [[ "$?" -eq 0 || "$startout" =~ .*already\ running.* ]]; Line 491: then Line 492: await_libvirt_start_workaround Line 493: return 0 -- To view, visit http://gerrit.ovirt.org/7331 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4f5207ac6ed7a9b01907b31d9ac7992aafb118ad Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Douglas Schilling Landgraf <dougsl...@redhat.com> Gerrit-Reviewer: Eduardo <ewars...@redhat.com> Gerrit-Reviewer: Igor Lvovsky <ilvov...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
