Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Date:   Thu, 7 Dec 2023 00:36:23 -0800
From:   Christoph Hellwig <hch@infradead.org>
To:     Sergei Shtepa <sergei.shtepa@linux.dev>
Cc:     axboe@kernel.dk, hch@infradead.org, corbet@lwn.net,
        snitzer@kernel.org, mingo@redhat.com, peterz@infradead.org,
        juri.lelli@redhat.com, viro@zeniv.linux.org.uk, brauner@kernel.org,
        linux-block@vger.kernel.org, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        Sergei Shtepa <sergei.shtepa@veeam.com>
Subject: Re: [PATCH v6 07/11] blksnap: difference storage and chunk
Message-ID: <ZXGEB5M1+/2QMxqi@infradead.org>
References: <20231124165933.27580-1-sergei.shtepa@linux.dev>
 <20231124165933.27580-8-sergei.shtepa@linux.dev>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231124165933.27580-8-sergei.shtepa@linux.dev>
Precedence: bulk

> +static inline bool unsupported_mode(const umode_t m)
> +{
> +	return (S_ISCHR(m) || S_ISFIFO(m) || S_ISSOCK(m));
> +}
> +
> +static inline bool unsupported_flags(const unsigned int flags)
> +{
> +	if (!(flags | O_RDWR)) {
> +		pr_err("Read and write access is required\n");
> +		return true;
> +	}
> +	if (!(flags | O_EXCL)) {
> +		pr_err("Exclusive access is required\n");
> +		return true;
> +	}

You probably want to positively check the allowed flags and types
to be more future proof.  I'd also drop these very easy to trigger
messages.

> +	if (S_ISBLK(file_inode(file)->i_mode)) {

Splitting the blk and regular file open path into separate helpers
would improve readability a lot I think.

> +		/*
> +		 * The block device is opened non-exclusively.
> +		 * It should be exclusive to open the file whose descriptor is
> +		 * passed to the module.
> +		 */
> +		bdev = blkdev_get_by_dev(dev_id,
> +					 BLK_OPEN_READ | BLK_OPEN_WRITE,
> +					 NULL, NULL);

Note that this will have some interesting interaction with the patches
from Jan to optionally disallow any other write for exclusively opened
block devices.  But given that right now we don't support using the
original device as backing store, this probably should become an
exclusive open anyway.