Re: [PATCH 03/21] parallels: invent parallels_opts_prealloc() helper to parse prealloc opts

[Denis V. Lunev]

On 9/17/23 13:40, Mike Maslenkin wrote:
> This is not introduced by this patch,
> but looks like qemu_opts_del(opts) missed.
nice spot!


> On Fri, Sep 15, 2023 at 9:41 PM Denis V. Lunev <den@openvz.org> wrote:
> > This patch creates above mentioned helper and moves its usage to the
> > beginning of parallels_open(). This simplifies parallels_open() a bit.
> >
> > The patch also ensures that we store prealloc_size on block driver state
> > always in sectors. This makes code cleaner and avoids wrong opinion at
> > the assignment that the value is in bytes.
> >
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
> > ---
> >   block/parallels.c | 65 +++++++++++++++++++++++++++--------------------
> >   1 file changed, 38 insertions(+), 27 deletions(-)
> >
> > diff --git a/block/parallels.c b/block/parallels.c
> > index 428f72de1c..1d5409f2ba 100644
> > --- a/block/parallels.c
> > +++ b/block/parallels.c
> > @@ -1025,6 +1025,38 @@ static int parallels_update_header(BlockDriverState *bs)
> >       return bdrv_pwrite_sync(bs->file, 0, size, s->header, 0);
> >   }
> >
> > +
> > +static int parallels_opts_prealloc(BlockDriverState *bs, QDict *options,
> > +                                   Error **errp)
> > +{
> > +    char *buf;
> > +    int64_t bytes;
> > +    BDRVParallelsState *s = bs->opaque;
> > +    Error *local_err = NULL;
> > +    QemuOpts *opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
> > +    if (!opts) {
> > +        return -ENOMEM;
> > +    }
> > +
> > +    if (!qemu_opts_absorb_qdict(opts, options, errp)) {
> > +        return -EINVAL;
> > +    }
> > +
> > +    bytes = qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
> > +    s->prealloc_size = bytes >> BDRV_SECTOR_BITS;
> qemu_opt_get_size_del returns uint64_t, so what's a reason to declare
> "bytes" variable  as int64_t
> and then shift it to the right?  I see here it can not be negative,
> but it's a common to use signed values and not to add explicit check
> before shifting to right In this file
> I takes time to ensure that initial values are not negative.
>
> Regards,
> Mike.

apparently no specific reason. Just all variables here
dealing with offsets are int64_t. Will change.

Thanks,
    Den