On Tue, Mar 26, 2019 at 5:07 PM Tzvetomir Stoyanov <tstoyanov@xxxxxxxxxx> wrote:
<snip>
> @@ -120,10 +146,13 @@ static const char *cmd_to_name(int cmd)
> struct tracecmd_msg {
> struct tracecmd_msg_header hdr;
> union {
> - struct tracecmd_msg_tinit tinit;
> - struct tracecmd_msg_rinit rinit;
> - struct tracecmd_msg_trace_req trace_req;
> - struct tracecmd_msg_trace_resp trace_resp;
> + struct tracecmd_msg_tinit tinit;
> + struct tracecmd_msg_rinit rinit;
> + struct tracecmd_msg_trace_req trace_req;
> + struct tracecmd_msg_trace_resp trace_resp;
> + struct tracecmd_msg_tsync_stop ts_stop;
> + struct tracecmd_msg_tsync_req ts_req;
> + struct tracecmd_msg_tsync_resp ts_resp;
> };
> union {
> struct tracecmd_msg_opt *opt;
> @@ -161,6 +190,7 @@ static int msg_write(int fd, struct tracecmd_msg *msg)
The second union in tracecmd_msg is already gone in master and this
series needs to get rebased.
<snip>
> diff --git a/tracecmd/trace-output.c b/tracecmd/trace-output.c
> index d57539c..fc338ca 100644
> --- a/tracecmd/trace-output.c
> +++ b/tracecmd/trace-output.c
> @@ -960,7 +960,7 @@ tracecmd_add_option(struct tracecmd_output *handle,
> {
> struct iovec vect;
>
> - vect.iov_base = data;
> + vect.iov_base = (void *)data;
> vect.iov_len = size;
> return tracecmd_add_option_v(handle, id, &vect, 1);
> }
This fix needs to be folded in patch 7 where this code is introduced
rather than here.
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 72a8ed3..14f3cf9 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -5609,6 +5631,42 @@ static bool has_local_instances(void)
> return false;
> }
>
> +//#define TSYNC_DEBUG
Remove? This define can be passed on the command line to make.
> +static void write_guest_time_shift(struct buffer_instance *instance)
> +{
> + struct tracecmd_output *handle;
> + struct iovec vector[3];
> + char *file;
> + int fd;
> +
> + if (!instance->time_sync_count)
> + return;
> +
> + file = get_guest_file(output_file, instance->name);
> + fd = open(file, O_RDWR);
> + if (fd < 0)
> + die("error opening %s", file);
> + put_temp_file(file);
> + handle = tracecmd_get_output_handle_fd(fd);
> + vector[0].iov_len = sizeof(instance->time_sync_count);
> + vector[0].iov_base = &instance->time_sync_count;
> + vector[1].iov_len = sizeof(long long) * instance->time_sync_count;
> + vector[1].iov_base = instance->time_sync_ts;
> + vector[2].iov_len = sizeof(long long) * instance->time_sync_count;
> + vector[2].iov_base = instance->time_sync_offsets;
> + tracecmd_add_option_v(handle, TRACECMD_OPTION_TIME_SHIFT, vector, 3);
This seems to be the only use of the newly added
tracecmd_add_option_v. I'm not convinced that having struct iovec
interface for tracecmd options is a good pattern since
a) the iovec concatenation in tracecmd_add_option_v adds an extra
allocation for the most common case when there's a single piece of
data passed
b) it never actually gets passed to readv/writev
c) the above is not really portable since there's no guarantee that
sizeof(int) = 4
I think defining a function here that massages time sync samples into
TIME_SHIFT option format (in a portable way) and dropping the patch
that adds tracecmd_add_option_v altogether is a better way to go.
<snip>
> diff --git a/tracecmd/trace-timesync.c b/tracecmd/trace-timesync.c
> new file mode 100644
> index 0000000..074c9d0
> --- /dev/null
> +++ b/tracecmd/trace-timesync.c
> @@ -0,0 +1,805 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019, VMware, Tzvetomir Stoyanov <tstoyanov@xxxxxxxxxx>
> + *
> + */
> +
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <arpa/inet.h>
> +#include <linux/vm_sockets.h>
> +#include "trace-local.h"
<snip>
> +
> +static void get_vsocket_params(int fd, unsigned int *lcid, unsigned int *lport,
> + unsigned int *rcid, unsigned int *rport)
> +{
> + struct sockaddr_vm addr;
> + socklen_t addr_len = sizeof(addr);
> +
> + memset(&addr, 0, sizeof(addr));
> + if (getsockname(fd, (struct sockaddr *)&addr, &addr_len))
> + return;
> + if (addr.svm_family != AF_VSOCK)
> + return;
> + *lport = addr.svm_port;
> + *lcid = addr.svm_cid;
> +
> + memset(&addr, 0, sizeof(addr));
> + addr_len = sizeof(addr);
> + if (getpeername(fd, (struct sockaddr *)&addr, &addr_len))
> + return;
> + if (addr.svm_family != AF_VSOCK)
> + return;
> + *rport = addr.svm_port;
> + *rcid = addr.svm_cid;
> +}
This function can silently ignore errors if getsockname/getpeername
fail or file descriptor happens to not be a vsocket. Can you change it
return error and have callers check for it?
Thank you Ceco,
-- Slavi
|