Mike,
Thank you for reviewing my patch.
Here is my comment.
> > - Write callback
> > - Check if there are enough spaces to write logs with QueryVariableInfo()
> > to avoid handling out of space situation. (It is suggested by
> > Matthew Garrett.)
> >
>
> I would prefer to see the exposing query_variable_info callback as a separate patch. The check in the patch looks good.
>
OK. I will make a separate patch.
> > - Remove a logic erasing existing entries.
> >
> > - Erase callback
> > - Freshly create a logic rather than sharing it with a write callback because
> > erasing entries doesn't need in a write callback.
> >
>
> This sort of change is a lot easier to review if you copy and paste the routine in a patch separate from this one.
>
I will separate it as well.
> > + /*
> > + * Check if there is a space enough to log.
> > + * size: a size of logging data
> > + * DUMP_NAME_LEN * 2: a maximum size of variable name
> > + *
>
> extra line
>
Will fix.
> > +
> > + efivars->ops->set_variable(efi_name, &vendor, PSTORE_EFI_ATTRIBUTES,
> > + size, psi->buf);
> > +
> > + spin_unlock(&efivars->lock);
> > +
> > + if (size)
> > + ret = efivar_create_sysfs_entry(efivars,
> > + utf16_strsize(efi_name,
> > + DUMP_NAME_LEN * 2),
> > + efi_name, &vendor);
>
> What is happening here? Why is size checked for non-zero?
>
I just copied an original code sharing a logic of write callback and erase callback..
But, if size is zero, we don't need to create a sysfs file because a set_variable service erases the entry.
It is defined in EFI specification.
So, I think this code is right.
> > +++ b/fs/pstore/inode.c
> > @@ -175,7 +175,8 @@ static int pstore_unlink(struct inode *dir, struct dentry *dentry)
> > struct pstore_private *p = dentry->d_inode->i_private;
> >
> > if (p->psi->erase)
> > - p->psi->erase(p->type, p->id, p->psi);
> > + p->psi->erase(p->type, p->id, dentry->d_inode->i_ctime,
> > + p->psi);
>
> This doesn't look right. What guarantees that the i_ctime means anything across reboots?
>
Ctime is persistent across reboots because ctime of pstore means the date that the recode was
originally stored. (See below.)
To do this, efi_pstore saves the ctime to variable name at writing time and passes it to pstore at reading time.
fs/pstore/inode.c:
<snip>
/*
* Make a regular file in the root directory of our file system.
* Load it up with "size" bytes of data from "buf".
* Set the mtime & ctime to the date that this record was originally stored.
*/
int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
char *data, size_t size, struct timespec time,
struct pstore_info *psi) {
struct dentry *root = pstore_sb->s_root;
struct dentry *dentry;
struct inode *inode;
int rc = 0;
char name[PSTORE_NAMELEN];
<snip>
Seiji
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
