Re: [PATCH] block: Use LVM tools for LV block device truncation

[Daniel P . Berrangé]

On Mon, Mar 11, 2024 at 06:40:44PM +0100, Alexander Ivanov wrote:
> If a block device is an LVM logical volume we can resize it using
> standard LVM tools.
> 
> In raw_co_truncate() check if the block device is a LV using lvdisplay
> and resize it executing lvresize.
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>  block/file-posix.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 35684f7e21..cf8a04e6f7 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2670,6 +2670,33 @@ static int coroutine_fn 
> raw_co_truncate(BlockDriverState *bs, int64_t offset,
>      if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
>          int64_t cur_length = raw_getlength(bs);
>  
> +        /*
> +         * Check if the device is an LVM logical volume and try to resize
> +         * it using LVM tools.
> +         */
> +        if (S_ISBLK(st.st_mode) && offset > 0) {
> +            char cmd[PATH_MAX + 32];
> +
> +            snprintf(cmd, sizeof(cmd), "lvdisplay %s > /dev/null",
> +                     bs->filename);

PATH_MAX + snprint is a bad practice - g_strdup_printf() is recommended
for dynamic allocation, along with g_autofree for release.

> +            ret = system(cmd);

IMHO using 'system' for spawning processes is dubious in any non-trivial
program.

Historically at least it does not have well defined behaviour wrt signal
handling in the child, before execve is called. ie potentially a signal
handler registered by QEMU in the parent could run in the child having
ill-effects.

'system' also executes via the shell which opens up many risks with
unsanitized files path being substituted into the command line.

> +            if (ret != 0) {
> +                error_setg(errp, "lvdisplay returned %d error for '%s'",
> +                           ret, bs->filename);
> +                return ret;
> +            }

Calling 'lvdisplay' doesn't seem to be needed. Surely 'lvresize' is
going to report errors if it isn't an LVM device.

If we want to detect an LVM device though, couldn't we lookup
'device-mapper'  in /proc/devices and then major the device major
node number.

> +
> +            snprintf(cmd, sizeof(cmd), "lvresize -f -L %ldB %s > /dev/null",
> +                     offset, bs->filename);
> +            ret = system(cmd);
> +            if (ret != 0) {
> +                error_setg(errp, "lvresize returned %d error for '%s'",
> +                           ret, bs->filename);

lvresize might display an message on stderr on failure but that's at best
going to QEMU's stderr. Any error needs to be captured and put into
this error message that's fed back to the user / QMP client.

> +            }
> +
> +            return ret;
> +        }
> +

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|