Re: [ethtool PATCH] FW dump support

On May 4, 2011, at 10:40 AM, Ben Hutchings wrote:

On Mon, 2011-05-02 at 16:29 -0700, anirban.chakraborty@qlogic.com wrote:

quoted

From: Anirban Chakraborty <redacted>

Added support to take FW dump via ethtool.

[...]

quoted

--- a/ethtool.c
+++ b/ethtool.c

[...]

quoted

@@ -263,6 +270,12 @@ static struct option {

		"Get Rx ntuple filters and actions\n" },
    { "-P", "--show-permaddr", MODE_PERMADDR,
		"Show permanent hardware address" },
+    { "-W", "--get-dump", MODE_GET_DUMP,
+		"Get dump level\n" },
+    { "-Wd", "--get-dump-data", MODE_GET_DUMP_DATA,
+		"Get dump data", "FILENAME " "Name of the dump file\n" },

The short options should only include one letter.  Also the general
pattern is that 'get' options use lower-case letters and 'set' options
use upper-case letters.  No, I'm not sure how best to handle a set of 3
options.  Maybe you can combine --get-dump and --get-dump-data, making
the filename optional?

Thanks for the info, I was not aware of it. Will address it.

quoted

+    { "-w", "--set-dump", MODE_SET_DUMP,
+		"Set dump level", "DUMPLEVEL " "Dump level for the device\n" },

The field this sets is described as 'flags' so does it consist of flags
or is it a level?

The idea is to have an opaque flag for the driver that it uses to set the dump level
(and hence the size) of it. I will use flag instead of level here so that there is no
ambiguity.

[...]

quoted

@@ -3241,6 +3270,86 @@ static int do_grxntuple(int fd, struct ifreq *ifr)

	return 0;
}

+static void do_writedump(struct ethtool_dump *dump)
+{
+	FILE *f = fopen(dump_file, "wb+");
+	size_t bytes;
+
+	if (!f ) {
+		fprintf(stderr, "Can't open file %s: %s\n",
+			dump_file, strerror(errno));
+		return;

On error, we must exit with code 1.

Will do.

quoted

+	}
+
+	bytes = fwrite(dump->data, 1, dump->len, f);
+	fclose(f);

[...]

These functions can also fail and need to be checked.  (Yes, fclose()
can fail, since it may have to flush buffered data.)

I agree. Will fix it. Thanks for the feedback.

-Anirban