Re: [PATCH 2/9] nbd-server, nbd-trdump: Add support for logging actual data
Hi Wouter,
On 3/3/22 10:34, Wouter Verhelst wrote:
On Fri, Jan 21, 2022 at 06:57:04PM +0100, Manfred Spraul wrote:
From: Manfred Spraul <manfred.spraul@de.bosch.com>
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 defines a new magic that is only used for the log file, and uses
an entry with that magic to store the information that the actual
data is stored in the trace file.
- 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.
- 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.
Open:
Should nbd-trdump abort when it sees an unknown new log config
option? Right now, it is only printed out as "UNKNOWN", but the
tool tries to continue anyways.
It might make sense to issue a warning in that case? Something along the
lines of:
"WARNING: unknown option found in log file. Will try to parse the log file, but
this may fail. You have been warned!"
Ok, I'll update the output.
+ break;
+ default:
+ printf("TRACE_OPTION ? Unknown type\n");
+ printf("Warning: Will try to parse the log file, but output may be
corrupted");
+ }
+ } else {
+ printf("TRACE_OPTION ? Unknown FROM_MAGIC\n");
+ printf("Warning: Will try to parse the log file, but output may be
corrupted");
+ }
+ break;
+
default:
printf("? Unknown transaction type %08x\n",magic);
+ printf("...");
break;
Known bugs:
Locking is missing. If multiple clients connect, then the data log
will be unusable.
Since this is mainly a debugging tool, I don't think that's a problem.
Here I would disagree (and I have already written the locking):
a) Nothing is worse than a debug tool with known issues. Noone would
trust the output, as it is not clear if it is valid or not.
b) Since I have added logging of the replies as well: From what I
understand, the replies are sent by worker threads. Thus even a single
client is enough to cause corrupt logs.
And, since sem_open() is allowed: it is not that difficult to implement.
--
Manfred
Reply to: