Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 17 Jul 2023 20:57:34 +0200
From:   Thomas =?utf-8?Q?Wei=C3=9Fschuh?= <thomas@t-8ch.de>
To:     Sergei Shtepa <sergei.shtepa@veeam.com>
Cc:     axboe@kernel.dk, hch@infradead.org, corbet@lwn.net,
        snitzer@kernel.org, viro@zeniv.linux.org.uk, brauner@kernel.org,
        dchinner@redhat.com, willy@infradead.org, dlemoal@kernel.org,
        jack@suse.cz, ming.lei@redhat.com, linux-block@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, Donald Buczek <buczek@molgen.mpg.de>
Subject: Re: [PATCH v5 04/11] blksnap: header file of the module interface
Message-ID: <822909b0-abd6-4e85-b739-41f8efa6feff@t-8ch.de>
References: <20230612135228.10702-1-sergei.shtepa@veeam.com>
 <20230612135228.10702-5-sergei.shtepa@veeam.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230612135228.10702-5-sergei.shtepa@veeam.com>
Precedence: bulk

On 2023-06-12 15:52:21+0200, Sergei Shtepa wrote:

> [..]

> diff --git a/include/uapi/linux/blksnap.h b/include/uapi/linux/blksnap.h
> new file mode 100644
> index 000000000000..2fe3f2a43bc5
> --- /dev/null
> +++ b/include/uapi/linux/blksnap.h
> @@ -0,0 +1,421 @@

> [..]

> +/**
> + * struct blksnap_snapshotinfo - Result for the command
> + *	&blkfilter_ctl_blksnap.blkfilter_ctl_blksnap_snapshotinfo.
> + *
> + * @error_code:
> + *	Zero if there were no errors while holding the snapshot.
> + *	The error code -ENOSPC means that while holding the snapshot, a snapshot
> + *	overflow situation has occurred. Other error codes mean other reasons
> + *	for failure.
> + *	The error code is reset when the device is added to a new snapshot.
> + * @image:
> + *	If the snapshot was taken, it stores the block device name of the
> + *	image, or empty string otherwise.
> + */
> +struct blksnap_snapshotinfo {
> +	__s32 error_code;
> +	__u8 image[IMAGE_DISK_NAME_LEN];

Nitpick:

Seems a bit weird to have a signed error code that is always negative.
Couldn't this be an unsigned number or directly return the error from
the ioctl() itself?

> +};
> +
> +/**
> + * DOC: Interface for managing snapshots
> + *
> + * Control commands that are transmitted through the blksnap module interface.
> + */
> +enum blksnap_ioctl {
> +	blksnap_ioctl_version,
> +	blksnap_ioctl_snapshot_create,
> +	blksnap_ioctl_snapshot_destroy,
> +	blksnap_ioctl_snapshot_append_storage,
> +	blksnap_ioctl_snapshot_take,
> +	blksnap_ioctl_snapshot_collect,
> +	blksnap_ioctl_snapshot_wait_event,
> +};
> +
> +/**
> + * struct blksnap_version - Module version.
> + *
> + * @major:
> + *	Version major part.
> + * @minor:
> + *	Version minor part.
> + * @revision:
> + *	Revision number.
> + * @build:
> + *	Build number. Should be zero.
> + */
> +struct blksnap_version {
> +	__u16 major;
> +	__u16 minor;
> +	__u16 revision;
> +	__u16 build;
> +};
> +
> +/**
> + * define IOCTL_BLKSNAP_VERSION - Get module version.
> + *
> + * The version may increase when the API changes. But linking the user space
> + * behavior to the version code does not seem to be a good idea.
> + * To ensure backward compatibility, API changes should be made by adding new
> + * ioctl without changing the behavior of existing ones. The version should be
> + * used for logs.
> + *
> + * Return: 0 if succeeded, negative errno otherwise.
> + */
> +#define IOCTL_BLKSNAP_VERSION							\
> +	_IOW(BLKSNAP, blksnap_ioctl_version, struct blksnap_version)

Shouldn't this be _IOR()?

  "_IOW means userland is writing and kernel is reading. _IOR
  means userland is reading and kernel is writing."

The other ioctl definitions seem to need a review, too.