[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 2/2] block: qcow2: remove the created file on initializati
From: |
Maxim Levitsky |
Subject: |
Re: [PATCH v3 2/2] block: qcow2: remove the created file on initialization error |
Date: |
Tue, 08 Dec 2020 19:11:45 +0200 |
User-agent: |
Evolution 3.36.5 (3.36.5-1.fc32) |
On Tue, 2020-12-08 at 19:54 +0300, Vladimir Sementsov-Ogievskiy wrote:
> 08.12.2020 19:27, Maxim Levitsky wrote:
> > On Tue, 2020-12-08 at 18:47 +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > 08.12.2020 17:21, Maxim Levitsky wrote:
> > > > If the qcow initialization fails, we should remove the file if it was
> > > > already created, to avoid leaving stale files around.
> > > >
> > > > We already do this for luks raw images.
> > > >
> > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > > ---
> > > > block/qcow2.c | 13 +++++++++++++
> > > > 1 file changed, 13 insertions(+)
> > > >
> > > > diff --git a/block/qcow2.c b/block/qcow2.c
> > > > index 3a90ef2786..3bc2096b72 100644
> > > > --- a/block/qcow2.c
> > > > +++ b/block/qcow2.c
> > > > @@ -3848,6 +3848,19 @@ static int coroutine_fn
> > > > qcow2_co_create_opts(BlockDriver *drv,
> > > > /* Create the qcow2 image (format layer) */
> > > > ret = qcow2_co_create(create_options, errp);
> > > > if (ret < 0) {
> > > > +
> > > > + Error *local_delete_err = NULL;
> > > > + int r_del = bdrv_co_delete_file(bs, &local_delete_err);
> > > > + /*
> > > > + * ENOTSUP will happen if the block driver doesn't support
> > > > + * the 'bdrv_co_delete_file' interface. This is a predictable
> > > > + * scenario and shouldn't be reported back to the user.
> > > > + */
> > > > + if ((r_del < 0) && (r_del != -ENOTSUP)) {
> > > > + error_report_err(local_delete_err);
> > > > + } else {
> > > > + error_free(local_delete_err);
> > > > + }
> > > > goto finish;
> > > > }
> > > >
> > > >
> > >
> > > Hi!
> > >
> > > As I understand, qcow2_co_create is a new interface and
> > > qcow2_co_create_opts() is old, and now works as a wrapper on
> > > qcow2_co_create.
> > >
> > > I think it's better to do the cleanup in qcow2_co_create, to bring the
> > > feature both to new and old interface in the same way.
> >
> > I think that the new interface doesn't need this fix, since
> > using the new interface is only possible from qmp which
> > forces the user to explicitly create and open the file
> > prior to formatting it with qcow2 format.
> >
>
> Oh yes, you are right. File is created by bdrv_create_file() in
> qcow2_co_create_opts() not in qcow2_co_create(). Still, I think, you should
> remove the file on any failure after bdrv_create_file() call, but you remove
> it only on the last failure point..
You are right! The bulk of the code that can fail is in qcow2_co_create_opts
but there
are indeed few error conditions prior to that.
Thanks for pointing that out.
I'll fix that.
Best regards,
Maxim Levitsky
>
>