From: Christoph Hellwig <hch@lst.de>
To: Kees Cook <keescook@chromium.org>
Cc: Christoph Hellwig <hch@lst.de>, Al Viro <viro@zeniv.linux.org.uk>,
WeiXiong Liao <gmpy.liaowx@gmail.com>,
axboe@kernel.dk, Anton Vorontsov <anton@enomsg.org>,
Colin Cross <ccross@android.com>, Tony Luck <tony.luck@intel.com>,
linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] pstore/blk: Use the normal block device I/O path
Date: Tue, 15 Jun 2021 14:31:18 +0200 [thread overview]
Message-ID: <20210615123118.GA14239@lst.de> (raw)
In-Reply-To: <20210614200421.2702002-1-keescook@chromium.org>
> - if (!dev || !dev->total_size || !dev->read || !dev->write)
> + if (!dev || !dev->total_size || !dev->read || !dev->write) {
> + if (!dev)
> + pr_err("NULL device info\n");
> + else {
> + if (!dev->total_size)
> + pr_err("zero sized device\n");
> + if (!dev->read)
> + pr_err("no read handler for device\n");
> + if (!dev->write)
> + pr_err("no write handler for device\n");
> + }
> return -EINVAL;
> + }
This is completely unrelated and should be a separate patch. And it
also looks rather strange, I'd at very least split the dev check out
and return early without the weird compound statement, but would probably
handle each one separate. All assuming that we really need all these
debug printks.
> /*
> * This takes its configuration only from the module parameters now.
> */
> static int __register_pstore_blk(void)
This needs a __init annotation now.
>
>
> {
> + struct pstore_device_info dev = {
> + .read = psblk_generic_blk_read,
> + .write = psblk_generic_blk_write,
> + };
On-stack method tables are a little odd..
> + if (!__is_defined(MODULE)) {
This looks a little weird. Can we define a rapper for this in config.h
that is a little more self-explanatory, e.g. in_module()?
> + if (!psblk_file->f_mapping)
> + pr_err("missing f_mapping\n");
Can't ever be true.
> + else if (!psblk_file->f_mapping->host)
> + pr_err("missing host\n");
Can't ever be true either.
> + else if (!I_BDEV(psblk_file->f_mapping->host))
> + pr_err("missing I_BDEV\n");
> + else if (!I_BDEV(psblk_file->f_mapping->host)->bd_inode)
> + pr_err("missing bd_inode\n");
І_BDEV just does pointer arithmetics, so it can't ever return NULL.
And there are no block device inodes without bd_inode either. And
all of this is per definition present for open S_ISBLK inodes.
next prev parent reply other threads:[~2021-06-15 12:31 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-14 20:04 [PATCH] pstore/blk: Use the normal block device I/O path Kees Cook
2021-06-14 20:09 ` Al Viro
2021-06-14 21:46 ` Kees Cook
2021-06-15 12:31 ` Christoph Hellwig [this message]
2021-06-15 16:04 ` Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210615123118.GA14239@lst.de \
--to=hch@lst.de \
--cc=anton@enomsg.org \
--cc=axboe@kernel.dk \
--cc=ccross@android.com \
--cc=gmpy.liaowx@gmail.com \
--cc=keescook@chromium.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tony.luck@intel.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).