Re: [PATCH 2/7] nbd-server, nbd-trdump: Add support for logging actual data
On Thu, Jan 13, 2022 at 08:34:02AM +0100, Manfred Spraul wrote:
> The datalog generated by nbd-server contains only the requests
> received by the server, not the actual data to be written.
>
> This patch adds support to write the actual data.
>
> As details:
> - It is configurable, the default behavior is not changed.
> - It adds a NBD_CMD_FLAG_DATALOG flag: I think this is the simplest
> way to mark that actual data wil follow in the log.
New protocol flags to be sent over the wire need to be documented in
doc/proto.md. Is your intent that a client can send this flag, or
that the flag is only used in the data log but never sent by a
compliant client and never seen by the server? If the former, then we
also need to figure out the best way for client and server to
negotiate when it is safe for the client to send the flag.
> As clients must not use a feature that is not exposed by the server,
> and as the server fails commands with unexpected flags, the approach
> should be safe. Obviously, if the protocol is extended so that 14
> flags are needed, then it will bite us.
It sounds like your intent is to just use the new flag only in the
log, and not in the NBD protocol itself. Seems like it might work.
> - It is an incompatible change: Current nbd-trdump utilities
> will just fail/produce bad output when called with a new
> log file, without a proper error message.
> - The change does not add a header to the datalog. I.e.:
> If we sometime in the future extend the log again, it will
> be again an incompatible change without a proper error
> message
> - nbd-trdump supports to dump also the messages sent by the
> server. Unfortunately, the current server does not log
> the sent messages. This change does not fix this.
>
> Known bugs:
> Locking is missing. If multiple clients connect, then the data log
> will be unusable.
>
> Plan: Use a named posix semaphore (sem_open()).
> Given the multi-process, multi-thread model, with a single fd shared
> by everyone, this is probably simpler than trying to find a reliable
> flock()/fcntl()/pthread_mutex_lock() combination.
> Alternative: shm_open()+a shared pthread_mutex.
>
> Signed-off-by: Manfred Spraul <manfred.spraul@de.bosch.com>
> ---
> nbd-helper.h | 4 ++++
> nbd-server.c | 20 +++++++++++++++++++-
> nbd-trdump.c | 13 +++++++++++++
> nbdsrv.h | 1 +
> 4 files changed, 37 insertions(+), 1 deletion(-)
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Reply to: