[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

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.

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;
  			printf("? Unknown transaction type %08x\n",magic);
+ printf("...");

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.



Reply to: