[PATCH 0/2] make all stored entries accessible.

[PATCH 0/2] make all stored entries accessible.

[Madper Xie]

1. checking type, id, psi, count and timespec when finding duplicate entries.
2. adding count and timestamp  for differentiating.

Madper Xie (2):
  pstore: avoid incorrectly mark entry as duplicate
  pstore: Differentiating names by adding count and timestamp

 fs/pstore/inode.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

-- 
1.8.4.2

[PATCH 1/2] pstore: avoid incorrectly mark entry as duplicate

[Madper Xie]

From: Madper Xie <bbboson@gmail.com>

pstore try to find duplicate entries by check both ID, type and psi.
They are not really enough for efi backend. dumped vars always have
the same type, psi and ID. like follows:
dump-type0-9-1-1382511508-C-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
dump-type0-9-1-1382515661-C-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0

The "duplicate" entries won't appear in pstorefs. And a complain will be
print -- pstore: failed to load 76 record(s) from 'efi'
So I add two more checks: timespec and count.

Signed-off-by: Madper Xie <cxie@redhat.com>
---
 fs/pstore/inode.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 1282384..0ae994c 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -47,6 +47,7 @@ static LIST_HEAD(allpstore);
 struct pstore_private {
 	struct list_head list;
 	struct pstore_info *psi;
+	struct timespec time;
 	enum pstore_type_id type;
 	u64	id;
 	int	count;
@@ -290,7 +291,9 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
 	list_for_each_entry(pos, &allpstore, list) {
 		if (pos->type == type &&
 		    pos->id == id &&
-		    pos->psi == psi) {
+		    pos->psi == psi &&
+		    pos->count == count &&
+		    !timespec_compare(&pos->time, &time)) {
 			rc = -EEXIST;
 			break;
 		}
@@ -312,6 +315,7 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
 	private->id = id;
 	private->count = count;
 	private->psi = psi;
+	memcpy(&private->time, &time, sizeof(struct timespec));
 
 	switch (type) {
 	case PSTORE_TYPE_DMESG:
-- 
1.8.4.2

RE: [PATCH 1/2] pstore: avoid incorrectly mark entry as duplicate

[Seiji Aguchi]

> -----Original Message-----
> From: Madper Xie [mailto:cxie@redhat.com]
> Sent: Wednesday, October 30, 2013 5:45 AM
> To: tony.luck@intel.com; keescook@chromium.org; ccross@android.com; anton@enomsg.org; Seiji Aguchi
> Cc: linux-efi@vger.kernel.org; linux-kernel@vger.kernel.org; bbboson@gmail.com; Madper Xie
> Subject: [PATCH 1/2] pstore: avoid incorrectly mark entry as duplicate
> 
> From: Madper Xie <bbboson@gmail.com>
> 
> pstore try to find duplicate entries by check both ID, type and psi.
> They are not really enough for efi backend. dumped vars always have
> the same type, psi and ID. like follows:
> dump-type0-9-1-1382511508-C-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
> dump-type0-9-1-1382515661-C-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
> 
> The "duplicate" entries won't appear in pstorefs. And a complain will be
> print -- pstore: failed to load 76 record(s) from 'efi'
> So I add two more checks: timespec and count.
> 
> Signed-off-by: Madper Xie <cxie@redhat.com>

Looks good to me.
Acked-by: Seiji Aguchi <seiji.aguchi@hds.com>

But, this patch has to be tested by other backend drivers like erst and ramoops.
In my understanding, erst has supported to log multiple events.
So, I'm interested why the same error hasn't happened in the other drivers.

Seiji

> ---
>  fs/pstore/inode.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index 1282384..0ae994c 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -47,6 +47,7 @@ static LIST_HEAD(allpstore);
>  struct pstore_private {
>  	struct list_head list;
>  	struct pstore_info *psi;
> +	struct timespec time;
>  	enum pstore_type_id type;
>  	u64	id;
>  	int	count;
> @@ -290,7 +291,9 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
>  	list_for_each_entry(pos, &allpstore, list) {
>  		if (pos->type == type &&
>  		    pos->id == id &&
> -		    pos->psi == psi) {
> +		    pos->psi == psi &&
> +		    pos->count == count &&
> +		    !timespec_compare(&pos->time, &time)) {
>  			rc = -EEXIST;
>  			break;
>  		}
> @@ -312,6 +315,7 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
>  	private->id = id;
>  	private->count = count;
>  	private->psi = psi;
> +	memcpy(&private->time, &time, sizeof(struct timespec));
> 
>  	switch (type) {
>  	case PSTORE_TYPE_DMESG:
> --
> 1.8.4.2

Re: [PATCH 1/2] pstore: avoid incorrectly mark entry as duplicate

[Madper Xie]

seiji.aguchi@hds.com writes:

>> -----Original Message-----
>> From: Madper Xie [mailto:cxie@redhat.com]
>> Sent: Wednesday, October 30, 2013 5:45 AM
>> To: tony.luck@intel.com; keescook@chromium.org; ccross@android.com; anton@enomsg.org; Seiji Aguchi
>> Cc: linux-efi@vger.kernel.org; linux-kernel@vger.kernel.org; bbboson@gmail.com; Madper Xie
>> Subject: [PATCH 1/2] pstore: avoid incorrectly mark entry as duplicate
>> 
>> From: Madper Xie <bbboson@gmail.com>
>> 
>> pstore try to find duplicate entries by check both ID, type and psi.
>> They are not really enough for efi backend. dumped vars always have
>> the same type, psi and ID. like follows:
>> dump-type0-9-1-1382511508-C-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
>> dump-type0-9-1-1382515661-C-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
>> 
>> The "duplicate" entries won't appear in pstorefs. And a complain will be
>> print -- pstore: failed to load 76 record(s) from 'efi'
>> So I add two more checks: timespec and count.
>> 
>> Signed-off-by: Madper Xie <cxie@redhat.com>
>
> Looks good to me.
> Acked-by: Seiji Aguchi <seiji.aguchi@hds.com>
>
> But, this patch has to be tested by other backend drivers like erst and ramoops.
> In my understanding, erst has supported to log multiple events.
> So, I'm interested why the same error hasn't happened in the other drivers.
>
> Seiji
Thanks for your review. I'll try to test other backends. :-)
>
>> ---
>>  fs/pstore/inode.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
>> index 1282384..0ae994c 100644
>> --- a/fs/pstore/inode.c
>> +++ b/fs/pstore/inode.c
>> @@ -47,6 +47,7 @@ static LIST_HEAD(allpstore);
>>  struct pstore_private {
>>  	struct list_head list;
>>  	struct pstore_info *psi;
>> +	struct timespec time;
>>  	enum pstore_type_id type;
>>  	u64	id;
>>  	int	count;
>> @@ -290,7 +291,9 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
>>  	list_for_each_entry(pos, &allpstore, list) {
>>  		if (pos->type == type &&
>>  		    pos->id == id &&
>> -		    pos->psi == psi) {
>> +		    pos->psi == psi &&
>> +		    pos->count == count &&
>> +		    !timespec_compare(&pos->time, &time)) {
>>  			rc = -EEXIST;
>>  			break;
>>  		}
>> @@ -312,6 +315,7 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
>>  	private->id = id;
>>  	private->count = count;
>>  	private->psi = psi;
>> +	memcpy(&private->time, &time, sizeof(struct timespec));
>> 
>>  	switch (type) {
>>  	case PSTORE_TYPE_DMESG:
>> --
>> 1.8.4.2


-- 
Best,
Madper Xie.

[PATCH 2/2] pstore: Differentiating names by adding count and timestamp

[Madper Xie]

From: Madper Xie <bbboson@gmail.com>

pstore denominates dumped file as type-psname-id. it makes many file have
the same name if there are many entries in backend have the same id.
So adding count and timestamp to file name for differentiating.

Signed-off-by: Madper Xie <cxie@redhat.com>
---
 fs/pstore/inode.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 0ae994c..36b502f 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -285,7 +285,7 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
 	int			rc = 0;
 	char			name[PSTORE_NAMELEN];
 	struct pstore_private	*private, *pos;
-	unsigned long		flags;
+	unsigned long		flags, timestamp;
 
 	spin_lock_irqsave(&allpstore_lock, flags);
 	list_for_each_entry(pos, &allpstore, list) {
@@ -316,35 +316,42 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
 	private->count = count;
 	private->psi = psi;
 	memcpy(&private->time, &time, sizeof(struct timespec));
+	timestamp = time.tv_sec;
 
 	switch (type) {
 	case PSTORE_TYPE_DMESG:
-		sprintf(name, "dmesg-%s-%lld%s", psname, id,
-						compressed ? ".enc.z" : "");
+		sprintf(name, "dmesg-%s-%lld-%d-%ld%s", psname, id, count,
+			timestamp, compressed ? ".enc.z" : "");
 		break;
 	case PSTORE_TYPE_CONSOLE:
-		sprintf(name, "console-%s", psname);
+		sprintf(name, "console-%s-%d-%ld", psname, count, timestamp);
 		break;
 	case PSTORE_TYPE_FTRACE:
-		sprintf(name, "ftrace-%s", psname);
+		sprintf(name, "ftrace-%s-%d-%ld", psname, count, timestamp);
 		break;
 	case PSTORE_TYPE_MCE:
-		sprintf(name, "mce-%s-%lld", psname, id);
+		sprintf(name, "mce-%s-%lld-%d-%ld", psname, id, count,
+			timestamp);
 		break;
 	case PSTORE_TYPE_PPC_RTAS:
-		sprintf(name, "rtas-%s-%lld", psname, id);
+		sprintf(name, "rtas-%s-%lld-%d-%ld", psname, id, count,
+			timestamp);
 		break;
 	case PSTORE_TYPE_PPC_OF:
-		sprintf(name, "powerpc-ofw-%s-%lld", psname, id);
+		sprintf(name, "powerpc-ofw-%s-%lld-%d-%ld", psname, id, count,
+			timestamp);
 		break;
 	case PSTORE_TYPE_PPC_COMMON:
-		sprintf(name, "powerpc-common-%s-%lld", psname, id);
+		sprintf(name, "powerpc-common-%s-%lld-%d-%ld", psname, id,
+			count, timestamp);
 		break;
 	case PSTORE_TYPE_UNKNOWN:
-		sprintf(name, "unknown-%s-%lld", psname, id);
+		sprintf(name, "unknown-%s-%lld-%d-%ld", psname, id, count,
+			timestamp);
 		break;
 	default:
-		sprintf(name, "type%d-%s-%lld", type, psname, id);
+		sprintf(name, "type%d-%s-%lld-%d-%ld", type, psname, id, count,
+			timestamp);
 		break;
 	}
 
-- 
1.8.4.2

RE: [PATCH 2/2] pstore: Differentiating names by adding count and timestamp

[Seiji Aguchi]

> -----Original Message-----
> From: Madper Xie [mailto:cxie@redhat.com]
> Sent: Wednesday, October 30, 2013 5:45 AM
> To: tony.luck@intel.com; keescook@chromium.org; ccross@android.com; anton@enomsg.org; Seiji Aguchi
> Cc: linux-efi@vger.kernel.org; linux-kernel@vger.kernel.org; bbboson@gmail.com; Madper Xie
> Subject: [PATCH 2/2] pstore: Differentiating names by adding count and timestamp
> 
> From: Madper Xie <bbboson@gmail.com>
> 
> pstore denominates dumped file as type-psname-id. it makes many file have
> the same name if there are many entries in backend have the same id.
> So adding count and timestamp to file name for differentiating.
> 
> Signed-off-by: Madper Xie <cxie@redhat.com>

It should be tested by other drivers as well..
But, looks good to me.
Acked-by: Seiji Aguchi <seiji.aguchi@hds.com>

> ---
>  fs/pstore/inode.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index 0ae994c..36b502f 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -285,7 +285,7 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
>  	int			rc = 0;
>  	char			name[PSTORE_NAMELEN];
>  	struct pstore_private	*private, *pos;
> -	unsigned long		flags;
> +	unsigned long		flags, timestamp;
> 
>  	spin_lock_irqsave(&allpstore_lock, flags);
>  	list_for_each_entry(pos, &allpstore, list) {
> @@ -316,35 +316,42 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
>  	private->count = count;
>  	private->psi = psi;
>  	memcpy(&private->time, &time, sizeof(struct timespec));
> +	timestamp = time.tv_sec;
> 
>  	switch (type) {
>  	case PSTORE_TYPE_DMESG:
> -		sprintf(name, "dmesg-%s-%lld%s", psname, id,
> -						compressed ? ".enc.z" : "");
> +		sprintf(name, "dmesg-%s-%lld-%d-%ld%s", psname, id, count,
> +			timestamp, compressed ? ".enc.z" : "");
>  		break;
>  	case PSTORE_TYPE_CONSOLE:
> -		sprintf(name, "console-%s", psname);
> +		sprintf(name, "console-%s-%d-%ld", psname, count, timestamp);
>  		break;
>  	case PSTORE_TYPE_FTRACE:
> -		sprintf(name, "ftrace-%s", psname);
> +		sprintf(name, "ftrace-%s-%d-%ld", psname, count, timestamp);
>  		break;
>  	case PSTORE_TYPE_MCE:
> -		sprintf(name, "mce-%s-%lld", psname, id);
> +		sprintf(name, "mce-%s-%lld-%d-%ld", psname, id, count,
> +			timestamp);
>  		break;
>  	case PSTORE_TYPE_PPC_RTAS:
> -		sprintf(name, "rtas-%s-%lld", psname, id);
> +		sprintf(name, "rtas-%s-%lld-%d-%ld", psname, id, count,
> +			timestamp);
>  		break;
>  	case PSTORE_TYPE_PPC_OF:
> -		sprintf(name, "powerpc-ofw-%s-%lld", psname, id);
> +		sprintf(name, "powerpc-ofw-%s-%lld-%d-%ld", psname, id, count,
> +			timestamp);
>  		break;
>  	case PSTORE_TYPE_PPC_COMMON:
> -		sprintf(name, "powerpc-common-%s-%lld", psname, id);
> +		sprintf(name, "powerpc-common-%s-%lld-%d-%ld", psname, id,
> +			count, timestamp);
>  		break;
>  	case PSTORE_TYPE_UNKNOWN:
> -		sprintf(name, "unknown-%s-%lld", psname, id);
> +		sprintf(name, "unknown-%s-%lld-%d-%ld", psname, id, count,
> +			timestamp);
>  		break;
>  	default:
> -		sprintf(name, "type%d-%s-%lld", type, psname, id);
> +		sprintf(name, "type%d-%s-%lld-%d-%ld", type, psname, id, count,
> +			timestamp);
>  		break;
>  	}
> 
> --
> 1.8.4.2

RE: [PATCH 0/2] make all stored entries accessible.

[Luck, Tony]

> 1. checking type, id, psi, count and timespec when finding duplicate entries.
> 2. adding count and timestamp  for differentiating.

Ah - I was expecting that the backend driver would have a unique "id" for
each record stored ... but is seems that this isn't true for efivars.

I just tried this patch series out with the erst backend. It seems to work, but
I was confused for a while by the filenames that I see in the pstore filesystem. There is a "--"
in the name that I couldn't quite figure out:

-r--r--r-- 1 root root 17498 Oct 30 13:41 dmesg-erst-5940651313304961025--2129078373-1383165669
-r--r--r-- 1 root root 17511 Oct 30 13:41 dmesg-erst-5940651313304961026--2129078373-1383165669
-r--r--r-- 1 root root 17530 Oct 30 13:41 dmesg-erst-5940651313304961027--2129078373-1383165669
-r--r--r-- 1 root root 17492 Oct 30 13:41 dmesg-erst-5940651313304961028--2129078373-1383165669
-r--r--r-- 1 root root 17499 Oct 30 13:41 dmesg-erst-5940651313304961029--2129078373-1383165669
-r--r--r-- 1 root root 17512 Oct 30 13:41 dmesg-erst-5940651313304961030--2129078373-1383165669
-r--r--r-- 1 root root 17531 Oct 30 13:41 dmesg-erst-5940651313304961031--2129078373-1383165669
-r--r--r-- 1 root root 17488 Oct 30 13:44 dmesg-erst-5940652283967569921--2129078373-1383165895
-r--r--r-- 1 root root 17512 Oct 30 13:44 dmesg-erst-5940652283967569922--2129078373-1383165895
-r--r--r-- 1 root root 17512 Oct 30 13:44 dmesg-erst-5940652283967569923--2129078373-1383165895
-r--r--r-- 1 root root 17500 Oct 30 13:44 dmesg-erst-5940652283967569924--2129078373-1383165895
-r--r--r-- 1 root root 17536 Oct 30 13:44 dmesg-erst-5940652283967569925--2129078373-1383165895
-r--r--r-- 1 root root 17513 Oct 30 13:44 dmesg-erst-5940652283967569926--2129078373-1383165895
-r--r--r-- 1 root root 17513 Oct 30 13:44 dmesg-erst-5940652283967569927--2129078373-1383165895
-r--r--r-- 1 root root 17501 Oct 30 13:44 dmesg-erst-5940652283967569928--2129078373-1383165895

The filename came from:
+               sprintf(name, "dmesg-%s-%lld-%d-%ld%s", psname, id, count,
+                       timestamp, compressed ? ".enc.z" : "");

So I guess that "count" is  -2129078373 - which is some uninitialized junk from the stack frame
of pstore_get_records() for the "int count" variable ... the erst reader function doesn't touch it.

I'll add an initializer "int count = 0;" there when applying the patches.

-Tony

RE: [PATCH 0/2] make all stored entries accessible.

[Seiji Aguchi]

> Ah - I was expecting that the backend driver would have a unique "id" for
> each record stored ... but is seems that this isn't true for efivars.
> 

So, do you mean efivars should fix to use the "id" in a proper way?

I acked Madper's patch 2/2 earlier today, but when I look at your test result, I'm not sure if
it is reasonable for users to make multiple numbers visible to the file name.

> -r--r--r-- 1 root root 17499 Oct 30 13:41 dmesg-erst-5940651313304961029--2129078373-1383165669

Seiji

RE: [PATCH 0/2] make all stored entries accessible.

[Luck, Tony]

> So, do you mean efivars should fix to use the "id" in a proper way?

It would avoid the need for all these tests, and additions to the filename to guarantee
uniqueness.

Not sure what options efivars has to create a unique, persistent "id" for each
record.  It's a fundamental part of how ERST works (though the unique ID is just
based around a timestamp).

> I acked Madper's patch 2/2 earlier today, but when I look at your test result, I'm not sure if
> it is reasonable for users to make multiple numbers visible to the file name.
>
>> -r--r--r-- 1 root root 17499 Oct 30 13:41 dmesg-erst-5940651313304961029--2129078373-1383165669

after I added the "count = 0" initialization the filename gets a tiny bit less
scary:

-r--r--r-- 1 root root 17499 Oct 30 13:41 dmesg-erst-5940651313304961029-0-1383165669

-Tony

Re: [PATCH 0/2] make all stored entries accessible.

[Madper Xie]

tony.luck@intel.com writes:

>> So, do you mean efivars should fix to use the "id" in a proper way?
>
> It would avoid the need for all these tests, and additions to the filename to guarantee
> uniqueness.
>
> Not sure what options efivars has to create a unique, persistent "id" for each
> record.  It's a fundamental part of how ERST works (though the unique ID is just
> based around a timestamp).
>
Okay, maybe there are three options here:
1. combine timestamp, count and part into "id".
   for now, in efi-pstore.c, *id = part. and we could simply change it
   to unique one. F.E. *id = (timestamp * 100 + part) * 100 + count.
2. change the id's type. let id become a string.
   so every backend could write anything to id. then it will become a
   part of filename in pstore filesystem. (but we need fix all backends
   since we modified api.)
3. apply the patches I have sent... even if the filename will be ugly
   and gory...
Which one do you prefer?
>> I acked Madper's patch 2/2 earlier today, but when I look at your test result, I'm not sure if
>> it is reasonable for users to make multiple numbers visible to the file name.
>>
>>> -r--r--r-- 1 root root 17499 Oct 30 13:41 dmesg-erst-5940651313304961029--2129078373-1383165669
>
> after I added the "count = 0" initialization the filename gets a tiny bit less
> scary:
>
> -r--r--r-- 1 root root 17499 Oct 30 13:41 dmesg-erst-5940651313304961029-0-1383165669
>
> -Tony


-- 
Best,
Madper Xie.

RE: [PATCH 0/2] make all stored entries accessible.

[Seiji Aguchi]

> 1. combine timestamp, count and part into "id".
>    for now, in efi-pstore.c, *id = part. and we could simply change it
>    to unique one. F.E. *id = (timestamp * 100 + part) * 100 + count.

My opinion close to 1.
But, the "*id" should not be the complex one like (timestamp * 100 + part) * 100 + count.
Rather, it should be a simple sequential number beginning with 1.

 - Remove "id" member from pstore_read_info struct.
 -  Introduce a global sequential counter like "static u64 efi_pstore_read_count" (or add the member to pstore_info structure?)
 - Initialize to "1" in efi_pstore_open().
 - Increment it in efi_pstore_read().

If we can do it, we don't need to touch a code of pstore filesystem and can avoid regressions of
other backend drivers.

Seiji

> -----Original Message-----
> From: linux-efi-owner@vger.kernel.org [mailto:linux-efi-owner@vger.kernel.org] On Behalf Of Madper Xie
> Sent: Wednesday, October 30, 2013 11:01 PM
> To: Luck, Tony
> Cc: Seiji Aguchi; Madper Xie; keescook@chromium.org; ccross@android.com; anton@enomsg.org; linux-efi@vger.kernel.org; linux-
> kernel@vger.kernel.org; bbboson@gmail.com
> Subject: Re: [PATCH 0/2] make all stored entries accessible.
> 
> 
> tony.luck@intel.com writes:
> 
> >> So, do you mean efivars should fix to use the "id" in a proper way?
> >
> > It would avoid the need for all these tests, and additions to the filename to guarantee
> > uniqueness.
> >
> > Not sure what options efivars has to create a unique, persistent "id" for each
> > record.  It's a fundamental part of how ERST works (though the unique ID is just
> > based around a timestamp).
> >
> Okay, maybe there are three options here:
> 1. combine timestamp, count and part into "id".
>    for now, in efi-pstore.c, *id = part. and we could simply change it
>    to unique one. F.E. *id = (timestamp * 100 + part) * 100 + count.
> 2. change the id's type. let id become a string.
>    so every backend could write anything to id. then it will become a
>    part of filename in pstore filesystem. (but we need fix all backends
>    since we modified api.)
> 3. apply the patches I have sent... even if the filename will be ugly
>    and gory...
> Which one do you prefer?
> >> I acked Madper's patch 2/2 earlier today, but when I look at your test result, I'm not sure if
> >> it is reasonable for users to make multiple numbers visible to the file name.
> >>
> >>> -r--r--r-- 1 root root 17499 Oct 30 13:41 dmesg-erst-5940651313304961029--2129078373-1383165669
> >
> > after I added the "count = 0" initialization the filename gets a tiny bit less
> > scary:
> >
> > -r--r--r-- 1 root root 17499 Oct 30 13:41 dmesg-erst-5940651313304961029-0-1383165669
> >
> > -Tony
> 
> 
> --
> Best,
> Madper Xie.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 0/2] make all stored entries accessible.

[Luck, Tony]

>> 1. combine timestamp, count and part into "id".
>>    for now, in efi-pstore.c, *id = part. and we could simply change it
>>    to unique one. F.E. *id = (timestamp * 100 + part) * 100 + count.
>
> My opinion close to 1.
> But, the "*id" should not be the complex one like (timestamp * 100 + part) * 100 + count.
> Rather, it should be a simple sequential number beginning with 1.

I also like option 1 ... but I think the "id" should be a persistent value for
a given saved record.  So some func(timestamp, part, count) would be a
good idea.  If we try using "sequential" numbers - and don't manage to
clear out /sys/fs/pstore each time - then we may have the same "dmesg"
file show up with different names on each boot.

Right now I have a simple script to save & clear ... not much more
complex than:

	cd /sys/fs/pstore
	cp * /var/log/save-pstore
	rm *

This depends on not re-using filenames (otherwise new files in pstore
might overwrite older saved files in my /var/log/save-pstore area).

-Tony

RE: [PATCH 0/2] make all stored entries accessible.

[Seiji Aguchi]

> I also like option 1 ... but I think the "id" should be a persistent value for
> a given saved record.  So some func(timestamp, part, count) would be a
> good idea.  If we try using "sequential" numbers - and don't manage to
> clear out /sys/fs/pstore each time - then we may have the same "dmesg"
> file show up with different names on each boot.
> 
> Right now I have a simple script to save & clear ... not much more
> complex than:
> 
> 	cd /sys/fs/pstore
> 	cp * /var/log/save-pstore
> 	rm *
> 
> This depends on not re-using filenames (otherwise new files in pstore
> might overwrite older saved files in my /var/log/save-pstore area).

I see.. It is a persuasive use case.

(1) I agree that some func(timestamp, part, count) would be  good idea.
      This might work, although an overflow will happen some time...
      sprintf(id_str, "%lld%d%d", timestamp, part, count)
      simple_str_to_ull(id_str, &id, base)
 
(2) There is a GetNextHighMonotonicCount() runtime service in EFI specification
       to get a persistent number across the reboot, but I'm not sure if it is safe to use it..
      Also,  it would be good if we can create the id by ourselves, rather than using firmware.

 (3) Also, (it might not be good idea),  if a pstore filesystem expects all backend drivers
       to use the persistent id, the pstore should provide it by itself.
       (by using time stamp counter or something like that.)

       As I looked into the ramoops's code. It seems to use a non-persistent sequential counter,by initializing
       read_cnt to "0" in ramoops_pstore_open(), and incrementing it in ramoops_pstore_read().
       It doesn't seem to be the pstore's expectation. 
       And when someone introduces a new driver, they may misunderstand how to create the id as well..

As above, there are mutiple ideas, but (1) is reasonable to me.    
 
Seiji