Re: [dbus PATCH 4/4] Introduce functions for translating Events to strings

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

 



On Thu, 2018-03-29 at 14:44 +0200, Pavel Hrdina wrote:
> On Thu, Mar 29, 2018 at 01:07:58PM +0200, Katerina Koukiou wrote:
> > The functions were copied from src/util/virutil.* files from
> > libvirt project.
> > 
> > They will be needed for other function of enum to string
> > as well.
> 
> This should be split into two patches, one that introduces the
> new macros and second one that uses them.
> 
> > Signed-off-by: Katerina Koukiou <kkoukiou@xxxxxxxxxx>
> > ---
> >  m4/virt-compile-warnings.m4 |  3 +++
> >  src/events.c                | 35 +------------------------------
> > ----
> >  src/util.c                  | 30 ++++++++++++++++++++++++++++++
> >  src/util.h                  | 30 ++++++++++++++++++++++++++++++
> >  test/test_connect.py        |  4 ++--
> >  test/test_domain.py         |  8 ++++----
> >  6 files changed, 70 insertions(+), 40 deletions(-)
> > 
> > diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-
> > warnings.m4
> > index 6ece136..7bc49b2 100644
> > --- a/m4/virt-compile-warnings.m4
> > +++ b/m4/virt-compile-warnings.m4
> > @@ -123,6 +123,9 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[
> >      # but need to rewrite various areas of code first
> >      wantwarn="$wantwarn -Wno-format-truncation"
> >  
> > +    # Needeed for *EventToString related functions.
> > +    wantwarn="$wantwarn -Wno-suggest-attribute=pure"
> > +
> 
> There is no need to disable this warning, we can use G_GNUC_PURE for
> the affected functions.
> 
> >      # This should be < 256 really. Currently we're down to 4096,
> >      # but using 1024 bytes sized buffers (mostly for virStrerror)
> >      # stops us from going down further
> > diff --git a/src/events.c b/src/events.c
> > index 62f3729..52c53ac 100644
> > --- a/src/events.c
> > +++ b/src/events.c
> > @@ -12,41 +12,8 @@ virtDBusEventsDomainLifecycle(virConnectPtr
> > connection G_GNUC_UNUSED,
> >                                gpointer opaque)
> >  {
> >      virtDBusConnect *connect = opaque;
> > -    const gchar *event_type = NULL;
> >      g_autofree gchar *path = NULL;
> >  
> > -    switch (event) {
> > -    case VIR_DOMAIN_EVENT_DEFINED:
> > -        event_type = "DomainDefined";
> > -        break;
> > -    case VIR_DOMAIN_EVENT_UNDEFINED:
> > -        event_type = "DomainUndefined";
> > -        break;
> > -    case VIR_DOMAIN_EVENT_STARTED:
> > -        event_type = "DomainStarted";
> > -        break;
> > -    case VIR_DOMAIN_EVENT_SUSPENDED:
> > -        event_type = "DomainSuspended";
> > -        break;
> > -    case VIR_DOMAIN_EVENT_RESUMED:
> > -        event_type = "DomainResumed";
> > -        break;
> > -    case VIR_DOMAIN_EVENT_STOPPED:
> > -        event_type = "DomainStopped";
> > -        break;
> > -    case VIR_DOMAIN_EVENT_SHUTDOWN:
> > -        event_type = "DomainShutdown";
> > -        break;
> > -    case VIR_DOMAIN_EVENT_PMSUSPENDED:
> > -        event_type = "DomainPMSuspended";
> > -        break;
> > -    case VIR_DOMAIN_EVENT_CRASHED:
> > -        event_type = "DomainCrashed";
> > -        break;
> > -    default:
> > -        return 0;
> > -    }
> > -
> >      path = virtDBusUtilBusPathForVirDomain(domain, connect-
> > >domainPath);
> >  
> >      g_dbus_connection_emit_signal(connect->bus,
> > @@ -54,7 +21,7 @@ virtDBusEventsDomainLifecycle(virConnectPtr
> > connection G_GNUC_UNUSED,
> >                                    connect->connectPath,
> >                                    VIRT_DBUS_CONNECT_INTERFACE,
> >                                    "Domain",
> > -                                  g_variant_new("(os)", path,
> > event_type),
> > +                                  g_variant_new("(os)", path,
> > virtDBusDomainEventToString(event)),
> >                                    NULL);
> >  
> >      return 0;
> > diff --git a/src/util.c b/src/util.c
> > index d6c27f3..9f645d2 100644
> > --- a/src/util.c
> > +++ b/src/util.c
> > @@ -124,3 +124,33 @@ virtDBusUtilVirDomainListFree(virDomainPtr
> > *domains)
> >  
> >      g_free(domains);
> >  }
> > +
> > +const gchar *virEnumToString(const gchar *const*types,
> > +                             guint ntypes,
> > +                             gint type)
> 
> Return type should be on a separate line, see HACKING.md and other
> functions in this file.
> 
> Even though that this function is copied from libvirt where we use
> vir* prefix, this package uses virtDBus* prefix and each file has
> it's own prefix, so in this case the prefix is virtDBusUtil* and the
> function name should be virtDBusUtilEnumToString.
> 
> > +{
> > +    if (type < 0 || (unsigned)type >= ntypes)
> > +        return NULL;
> > +
> > +    return types[type];
> > +}
> > +
> > +VIR_ENUM_DECL(virtDBusDomainEvent)
> > +VIR_ENUM_IMPL(virtDBusDomainEvent,
> > +              VIR_DOMAIN_EVENT_LAST,
> > +              "Defined",
> > +              "Undefined",
> > +              "Started",
> > +              "Suspended",
> > +              "Resumed",
> > +              "Stopped",
> > +              "Shutdown",
> > +              "PMSuspended",
> > +              "Crashed")
> 
> This is currently used only in event.c so it should go into top of
> that
> file.
> 
> > +
> > +const gchar *
> > +virtDBusDomainEventToString(gint event)
> > +{
> > +    const gchar *str = virtDBusDomainEventTypeToString(event);
> > +    return str ? str : "unknown";
> > +}
> 
> This one as well and the function name prefix should be fixed.
> 
> > diff --git a/src/util.h b/src/util.h
> > index 4304bac..22cf25e 100644
> > --- a/src/util.h
> > +++ b/src/util.h
> > @@ -2,6 +2,7 @@
> >  
> >  #include "gdbus.h"
> >  
> > +#define VIR_ENUM_SENTINELS
> >  #include <libvirt/libvirt.h>
> >  
> >  #define VIRT_DBUS_ERROR virtDBusErrorQuark()
> > @@ -37,3 +38,32 @@ virtDBusUtilVirDomainListFree(virDomainPtr
> > *domains);
> >  
> >  G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomain, virDomainFree);
> >  G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainPtr,
> > virtDBusUtilVirDomainListFree);
> > +
> > +gint virEnumFromString(const gchar *const*types,
> > +                       guint ntypes,
> > +                       const gchar *type);
> > +
> > +const gchar *virEnumToString(const gchar *const*types,
> > +                             guint ntypes,
> > +                             gint type);
> 
> Same comment for the return type and the function name.
> 
> > +
> > +# define VIR_ENUM_IMPL(name, lastVal, ...) \
> > +    static const gchar *const name ## TypeList[] = { __VA_ARGS__
> > }; \
> > +    G_STATIC_ASSERT(G_N_ELEMENTS(name ## TypeList) == lastVal); \
> > +    const gchar *name ## TypeToString(int type) { \
> > +        return virEnumToString(name ## TypeList, \
> > +                               G_N_ELEMENTS(name ## TypeList), \
> > +                               type); \
> > +    } \
> > +    gint name ## TypeFromString(const gchar *type) { \
> > +        return virEnumFromString(name ## TypeList, \
> > +                                 G_N_ELEMENTS(name ## TypeList), \
> > +                                 type); \
> > +    }
> > +
> > +# define VIR_ENUM_DECL(name) \
> > +    const gchar *name ## TypeToString(gint type); \
> > +    gint name ## TypeFromString(const gchar*type);
> 
> There is no need to have the space after '#', that is used in libvirt
> because the macro is in #ifndef ... #endif block.
> 
> Both macros should use also the VIRT_DBUS prefix instead of VIR.
> 
> Pavel

All comments fixed in the second patchset.

Thanks,

Katerina 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux