D19514: Add support for detecting avahi crash - cleanup invalid dbus objects in that case

[Harald Sitter]

sitter added a comment.


  I haven't had a proper look, so only style complaints for now... but...
  
  Isn't the problem rather that the daemon shouldn't be randomly restarted on a live system? I.e. this ultimately is an integration issue on the distro-level.
  I doubt many if any avahi clients handle this properly and disadvantageous side effects are rather expected. Same as pulseaudio for example.
  
  So I am not sure if this is worth handling at all, but if we handle it we should do it properly and republish/rediscover services. The client shouldn't get broken frontend objects just because the backend of the backend restarted.

INLINE COMMENTS

> CMakeLists.txt:22
>          avahi_listener.cpp
> +        avahi-disconn-handler.cpp
>      )

disconn**ect**

> avahi-disconn-handler.cpp:1
> +#include <QDBusServiceWatcher>
> +#include <QDBusConnection>

needs a license header

> avahi-disconn-handler.cpp:8
> +    auto nameChangeObserver = new QDBusServiceWatcher("org.freedesktop.Avahi", QDBusConnection::systemBus(), QDBusServiceWatcher::WatchForOwnerChange, this);
> +    QObject::connect(nameChangeObserver, &QDBusServiceWatcher::serviceOwnerChanged, this,
> +                     [this](const QString& name, const QString& oldO, const QString& newO) {

no need for QObject::

> avahi-disconn-handler.cpp:9
> +    QObject::connect(nameChangeObserver, &QDBusServiceWatcher::serviceOwnerChanged, this,
> +                     [this](const QString& name, const QString& oldO, const QString& newO) {
> +                         if (newO.isEmpty())

`&` go right of the space

use descriptive variable names

> avahi-disconn-handler.cpp:15
> +
> +auto KDNSSD::AvahiDisconnectHandler::instance() -> AvahiDisconnectHandler&
> +{

don't use trailing return type but simply use return type instead of auto

> avahi-disconn-handler.h:1
> +#ifndef AVAHI_DISCONNECT_HANDLER___H_
> +#define AVAHI_DISCONNECT_HANDLER___H_

needs a license header

> avahi-disconn-handler.h:8
> +
> +  class AvahiDisconnectHandler : public QObject
> +  {

we do not indent classes within a namespace

> avahi-disconn-handler.h:12
> +
> +      AvahiDisconnectHandler();
> +  Q_SIGNALS:

privates at the bottom of the class in a private section please

> avahi-disconn-handler.h:13
> +      AvahiDisconnectHandler();
> +  Q_SIGNALS:
> +      void avahiDisconnected();

the signal section goes after the public section

> avahi-domainbrowser.cpp:40
> +                     [this]() {
> +                         delete d->m_browser;
> +                         d->m_browser = 0;

deleteLater may be better

the cleanup in this slot seems insufficient compared to the cleanup done for ServiceBrowser?

> avahi-remoteservice.cpp:37
>  {
> +  QObject::connect(&AvahiDisconnectHandler::instance (), &AvahiDisconnectHandler::avahiDisconnected, this,
> +                   [this]() {

no need for QObject::

goes for all? other classes as well

> avahi-remoteservice.cpp:40
> +                       K_D;
> +                       if(d->m_resolver) {
> +                           delete d->m_resolver;

space after if

> avahi-servicebrowser.cpp:42
>      d->m_timer.setSingleShot(true);
> +    QObject::connect(&AvahiDisconnectHandler::instance (), &AvahiDisconnectHandler::avahiDisconnected, d, [this]() { d->clean(); });
>  }

no space after instance

also I think you need to bind the connection to `this` seeing as you are capturing `this`

> avahi-servicebrowser.cpp:42
>      d->m_timer.setSingleShot(true);
> +    QObject::connect(&AvahiDisconnectHandler::instance (), &AvahiDisconnectHandler::avahiDisconnected, d, [this]() { d->clean(); });
>  }

no need for QObject::

> avahi-servicebrowser_p.h:38
>      friend class ServiceBrowser; // So the public class may functor connect.
> +    void clean();
>  public:

put this in an actual private section at the bottom please

> avahi-servicetypebrowser.cpp:37
>      d->m_timer.setSingleShot(true);
> +    QObject::connect(&AvahiDisconnectHandler::instance(), &AvahiDisconnectHandler::avahiDisconnected, this,
> +                     [this]() {

no need for QObject::

> avahi-servicetypebrowser.cpp:39
> +                     [this]() {
> +                         delete d->m_browser;
> +                         d->m_browser = 0;

deleteLater may be better seeing as this is a slot and deletes in slots are dangerous

> avahi-servicetypebrowser.cpp:40
> +                         delete d->m_browser;
> +                         d->m_browser = 0;
> +                     });

nullptr

> avahi_listener.cpp:28
>  {
> +    m_connection = QObject::connect(&AvahiDisconnectHandler::instance(), &AvahiDisconnectHandler::avahiDisconnected,
> +                                    [this]() {

use a member initializer list here

> avahi_listener.cpp:32
> +                                    });
>  }
>  

The other classes ctors look super repetitive and could maybe just be moved in here. couldn't you simply make a pure virtual `reset()` in here that is implemented in all avahi listeners and connect to that? Specifically since the expectation is that every dbus based object switches state when avahi disappears/appears the best way to express that in a way that one doesn't forget about it moving forward would be making the compiler throw a tantrum on a not implemented reset functionality.

> avahi_listener_p.h:26
>  #include <QString>
> +#include <QMetaObject>
>  

sort alphabetically

> avahi_listener_p.h:37
>  {
> +    QMetaObject::Connection m_connection;
>  public:

not very descriptive name

privates at the bottom of the class in a private section please

REPOSITORY
  R272 KDNSSD

REVISION DETAIL
  https://phabricator.kde.org/D19514

To: jpalecek, sitter
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190304/619526e1/attachment-0001.html>