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

[Alexander Ivanov]

Thank you for the review.

On 3/11/24 19:24, Daniel P. Berrangé wrote:
> 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.
The problem is that we don't know if 'lvresize' returned an error because of
non-LVM device or there was another reason. For the first variant we should
continue original code execution, for the second - return an error.
> 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.
It will require more code for getting device major, file reading and 
parsing.
Why this way is better?
> > +
> > +            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.
It seems I need to implement a high level function for programs 
execution. Looked at
g_spawn_sync(), but it uses fork() under the hood and we could have a 
performance
issue. Haven't found a high level function that uses vfork() and allows 
to catch stderr.

> > +            }
> > +
> > +            return ret;
> > +        }
> > +
> With regards,
> Daniel

--
Best regards,
Alexander Ivanov