[for-1.4,1/2] trace: use glib atomic int types

Juan reported that RHEL 6.4 hosts give compiler warnings because we use
unsigned int while glib prototypes use volatile gint in trace/simple.c.

  trace/simple.c:223: error: pointer targets in passing argument 1 of 'g_atomic_int_compare_and_exchange' differ in signedness

These variables are only accessed with glib atomic int functions so
let's play it by the book and use volatile gint.

Reported-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 trace/simple.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Stefan Hajnoczi <stefanha@redhat.com> writes:

> Juan reported that RHEL 6.4 hosts give compiler warnings because we use
> unsigned int while glib prototypes use volatile gint in trace/simple.c.
>
>   trace/simple.c:223: error: pointer targets in passing argument 1 of 'g_atomic_int_compare_and_exchange' differ in signedness

Meh.  Contrary to documentation and how current GLib versions behave, in
other words a bug in need of a workaround.

> These variables are only accessed with glib atomic int functions so
> let's play it by the book and use volatile gint.

gint is a silly alias for int, and used completely interchangeably, even
within GLib APIs.  Any pretentions of treating it as something more
abstract break down at the first printf(), if not earlier.  But if you
think it helps...

I doubt volatile has any effect here.

> Reported-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  trace/simple.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/trace/simple.c b/trace/simple.c
> index 74701e3..1d5d8e4 100644
> --- a/trace/simple.c
> +++ b/trace/simple.c
> @@ -51,9 +51,9 @@ enum {
>  };
>  
>  uint8_t trace_buf[TRACE_BUF_LEN];
> -static unsigned int trace_idx;
> +static volatile gint trace_idx;
>  static unsigned int writeout_idx;
> -static int dropped_events;
> +static volatile gint dropped_events;
>  static FILE *trace_fp;
>  static char *trace_file_name;
>  
> @@ -267,7 +267,7 @@ void trace_record_finish(TraceBufferRecord *rec)
>      record.event |= TRACE_RECORD_VALID;
>      write_to_buffer(rec->tbuf_idx, &record, sizeof(TraceRecord));
>  
> -    if ((g_atomic_int_get(&trace_idx) - writeout_idx)
> +    if (((unsigned int)g_atomic_int_get(&trace_idx) - writeout_idx)
>          > TRACE_BUF_FLUSH_THRESHOLD) {
>          flush_trace_file(false);
>      }

Unchanged:

int trace_record_start(TraceBufferRecord *rec, TraceEventID event, size_t datasize)
{
    unsigned int idx, rec_off, old_idx, new_idx;
    uint32_t rec_len = sizeof(TraceRecord) + datasize;
    uint64_t timestamp_ns = get_clock();

    do {
        old_idx = g_atomic_int_get(&trace_idx);
        smp_rmb();
        new_idx = old_idx + rec_len;

        if (new_idx - writeout_idx > TRACE_BUF_LEN) {
            /* Trace Buffer Full, Event dropped ! */
            g_atomic_int_inc(&dropped_events);
            return -ENOSPC;
        }
    } while (!g_atomic_int_compare_and_exchange(&trace_idx, old_idx, new_idx));
[...]
}

Now mixes int trace_idx with unsigned old_idx, new_idx.  No biggie, but
you might want to clean it up anyway.

Markus Armbruster <armbru@redhat.com> wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
>
>> Juan reported that RHEL 6.4 hosts give compiler warnings because we use
>> unsigned int while glib prototypes use volatile gint in trace/simple.c.
>>
>>   trace/simple.c:223: error: pointer targets in passing argument 1
>> of 'g_atomic_int_compare_and_exchange' differ in signedness
>
> Meh.  Contrary to documentation and how current GLib versions behave, in
> other words a bug in need of a workaround.
>
>> These variables are only accessed with glib atomic int functions so
>> let's play it by the book and use volatile gint.
>
> gint is a silly alias for int, and used completely interchangeably, even
> within GLib APIs.  Any pretentions of treating it as something more
> abstract break down at the first printf(), if not earlier.  But if you
> think it helps...

>> -static unsigned int trace_idx;
>> +static volatile gint trace_idx;

Problem was this bit.  int vs unsigned.

Later, Juan.

On Wed, Feb 13, 2013 at 01:25:02PM +0100, Juan Quintela wrote:
> Markus Armbruster <armbru@redhat.com> wrote:
> > Stefan Hajnoczi <stefanha@redhat.com> writes:
> >
> >> Juan reported that RHEL 6.4 hosts give compiler warnings because we use
> >> unsigned int while glib prototypes use volatile gint in trace/simple.c.
> >>
> >>   trace/simple.c:223: error: pointer targets in passing argument 1
> >> of 'g_atomic_int_compare_and_exchange' differ in signedness
> >
> > Meh.  Contrary to documentation and how current GLib versions behave, in
> > other words a bug in need of a workaround.
> >
> >> These variables are only accessed with glib atomic int functions so
> >> let's play it by the book and use volatile gint.
> >
> > gint is a silly alias for int, and used completely interchangeably, even
> > within GLib APIs.  Any pretentions of treating it as something more
> > abstract break down at the first printf(), if not earlier.  But if you
> > think it helps...
> 
> >> -static unsigned int trace_idx;
> >> +static volatile gint trace_idx;
> 
> Problem was this bit.  int vs unsigned.

We only access the variable through the glib API, never directly.  So in
this case I don't see much benefit in using nicer types, better to
conform precisely to the function declaration.

Stefan