On Fri, 31 Jan 2020 09:53:41 +0000
Tzvetomir Stoyanov <tstoyanov@xxxxxxxxxx> wrote:
> On Sat, Dec 21, 2019 at 6:48 AM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> >
> [ .. ]
> > > +#define PROTO_MASK_SIZE (sizeof(char))
> >
> > I'm thinking that declaring a mask the size of 1 is a bit overkill.
> >
> This define is a leftover from the previous implementation of this
> bitmask, where
> the size was 4 bytes. It was very easy to switch to 1 byte using the
> define, that's
> why I would prefer to keep it - in case we decide to change the size again.
> The code below is written with the assumption that the size could be
> more than 1 byte.
OK, that makes more sense.
>
> [ .. ]
> > > + */
> > > +void tracecmd_tsync_free(struct tracecmd_time_sync *tsync)
> > > +{
> > > + struct clock_sync_context *tsync_context;
> > > + struct tsync_proto *proto;
> > > +
> > > + if (!tsync->context)
> > > + return;
> > > + tsync_context = (struct clock_sync_context *)tsync->context;
> > > +
> > > + proto = tsync_proto_find(tsync->sync_proto);
> > > + if (proto && proto->clock_sync_free)
> > > + proto->clock_sync_free(tsync);
> > > +
> > > + clock_synch_delete_instance(tsync_context->vinst);
> > > + tsync_context->vinst = NULL;
> > > +
> > > + free(tsync_context->sync_ts);
> > > + free(tsync_context->sync_offsets);
> > > + tsync_context->sync_ts = NULL;
> > > + tsync_context->sync_offsets = NULL;
> > > + tsync_context->sync_count = 0;
> > > + tsync_context->sync_size = 0;
> > > + pthread_mutex_destroy(&tsync->lock);
> > > + pthread_cond_destroy(&tsync->cond);
> > > + free(tsync->clock_str);
> >
> > I would think we would want a free(tsync) here. As the name of the
> > function suggests.
> >
> There is no API to allocate it, that's why this function does not free
> the memory. There is one use case where this memory is not allocated,
> and free(tsync) will not work for it.
Then perhaps we should call it: tracecmd_tsync_cleanup(), as that's
what it is doing. _free() usually means you are actually freeing what
you pass in.
>
> [ .. ]
> > > +
> > > +unsigned int tracecmd_guest_tsync(char *tsync_protos,
> > > + unsigned int tsync_protos_size, char *clock,
> > > + unsigned int *tsync_port, pthread_t *thr_id)
> > > +{
> > > + struct tracecmd_time_sync *tsync = NULL;
> > > + cpu_set_t *pin_mask = NULL;
> > > + pthread_attr_t attrib;
> > > + size_t mask_size = 0;
> > > + unsigned int proto;
> > > + int ret;
> > > + int fd;
> > > +
> > > + fd = -1;
> > > + proto = tracecmd_tsync_proto_select(tsync_protos, tsync_protos_size);
> > > + if (!proto)
> > > + return 0;
> > > +#ifdef VSOCK
> > > + fd = trace_make_vsock(VMADDR_PORT_ANY);
> > > + if (fd < 0)
> > > + goto error;
> > > +
> > > + ret = trace_get_vsock_port(fd, tsync_port);
> > > + if (ret < 0)
> > > + goto error;
> > > +#else
> > > + return 0;
> >
> > If we have no synchronization support, shouldn't this return an error?
>
> This function returns the id of negotiated time sync protocol. 0 means the
> negotiation was not successful, no protocol is selected. When the caller
> receives 0, it means we have no synchronization with the peer.
OK, so this function definitely needs a "kernel doc" header explaining
this.
-- Steve
|