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
