| Message ID | 20201208142159.492489-3-mlevitsk@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qcow2: don't leave partially initialized file on image creation | expand |
On Tue 08 Dec 2020 03:21:59 PM CET, 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> Reviewed-by: Alberto Garcia <berto@igalia.com> > ret = qcow2_co_create(create_options, errp); > if (ret < 0) { > + > + Error *local_delete_err = NULL; Why that empty line though? Berto
On Tue, 2020-12-08 at 16:26 +0100, Alberto Garcia wrote: > On Tue 08 Dec 2020 03:21:59 PM CET, 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> > > Reviewed-by: Alberto Garcia <berto@igalia.com> > > > ret = qcow2_co_create(create_options, errp); > > if (ret < 0) { > > + > > + Error *local_delete_err = NULL; > > Why that empty line though? I didn't notice. I can send a new version if this is needed. Thanks for the review! Best regards, Maxim Levitsky > > Berto >
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.
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. Thus it is logical to make the user remove it as well if creation fails. Best regards, Maxim Levitsky > >
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..
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 > >
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; }
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(+)