| Message ID | 20190326211742.26140-2-danielhb413@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | delete created files when block_crypto_co_create_opts_luks fails | expand |
It looks like this one has gone un-noticed for a little while. On 3/26/19 5:17 PM, Daniel Henrique Barboza wrote: > Adding to Block Drivers the capability of being able to clean up > its created files can be useful in certain situations. For the > LUKS driver, for instance, a failure in one of its authentication > steps can leave files in the host that weren't there before. > > This patch adds the 'bdrv_co_delete_file' interface to block > drivers and add it to the 'file' driver in file-posix.c. The > implementation is given by 'raw_co_delete_file'. The helper > 'bdrv_path_is_regular_file' is being used only in raw_co_delete_file > at this moment, but it will be used inside LUKS in a later patch. > Foreseeing this future use, let's put it in block.c and make it > public. > > Suggested-by: Daniel P. Berrangé <berrange@redhat.com> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > block.c | 11 +++++++++++ > block/file-posix.c | 28 ++++++++++++++++++++++++++++ > include/block/block.h | 1 + > include/block/block_int.h | 6 ++++++ > 4 files changed, 46 insertions(+) > > diff --git a/block.c b/block.c > index 0a93ee9ac8..227362b282 100644 > --- a/block.c > +++ b/block.c > @@ -621,6 +621,17 @@ int get_tmp_filename(char *filename, int size) > #endif > } > > +/** > + * Helper that checks if a given string represents a regular > + * local file. > + */ > +bool bdrv_path_is_regular_file(const char *path) > +{ > + struct stat st; > + > + return (stat(path, &st) == 0) && S_ISREG(st.st_mode); > +} > + > /* > * Detect host devices. By convention, /dev/cdrom[N] is always > * recognized as a host CDROM. > diff --git a/block/file-posix.c b/block/file-posix.c > index d102f3b222..09d84bab37 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -2342,6 +2342,33 @@ static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts, > return raw_co_create(&options, errp); > } > > +/** > + * Co-routine function that erases a regular file. > + */ > +static int coroutine_fn raw_co_delete_file(const char *filename, > + Error **errp) Do we need to mark functions that make no use of coroutines as coroutine_fn? I guess this way the interface is *allowed* to be a coroutine if other drivers need to make use of that. > +{ > + int ret; > + > + /* Skip file: protocol prefix */ > + strstart(filename, "file:", &filename); > + This sticks out as fragile to me, but I guess that's exactly how create works, so... OK. > + if (!bdrv_path_is_regular_file(filename)) { > + ret = -ENOENT; > + error_setg_errno(errp, -ret, "%s is not a regular file", filename); > + goto done; > + } > + > + ret = unlink(filename); > + if (ret < 0) { > + ret = -errno; > + error_setg_errno(errp, -ret, "Error when deleting file %s", filename); > + } > + > +done: > + return ret; > +} > + > /* > * Find allocation range in @bs around offset @start. > * May change underlying file descriptor's file offset. > @@ -2867,6 +2894,7 @@ BlockDriver bdrv_file = { > .bdrv_co_block_status = raw_co_block_status, > .bdrv_co_invalidate_cache = raw_co_invalidate_cache, > .bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes, > + .bdrv_co_delete_file = raw_co_delete_file, > > .bdrv_co_preadv = raw_co_preadv, > .bdrv_co_pwritev = raw_co_pwritev, > diff --git a/include/block/block.h b/include/block/block.h > index e452988b66..820643f96d 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -363,6 +363,7 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base, > Error **errp); > void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base); > > +bool bdrv_path_is_regular_file(const char *path); > > typedef struct BdrvCheckResult { > int corruptions; > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 01e855a066..74abb78ce7 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -309,6 +309,12 @@ struct BlockDriver { > */ > int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs); > > + /* > + * Delete a local created file. > + */ > + int coroutine_fn (*bdrv_co_delete_file)(const char *filename, > + Error **errp); > + > /* > * Flushes all data that was already written to the OS all the way down to > * the disk (for example file-posix.c calls fsync()). > Seems alright at a glance, if we want this interface. Kevin, do we? --js
Hi John, On 5/13/19 4:47 PM, John Snow wrote: > It looks like this one has gone un-noticed for a little while. > > On 3/26/19 5:17 PM, Daniel Henrique Barboza wrote: >> Adding to Block Drivers the capability of being able to clean up >> its created files can be useful in certain situations. For the >> LUKS driver, for instance, a failure in one of its authentication >> steps can leave files in the host that weren't there before. >> >> This patch adds the 'bdrv_co_delete_file' interface to block >> drivers and add it to the 'file' driver in file-posix.c. The >> implementation is given by 'raw_co_delete_file'. The helper >> 'bdrv_path_is_regular_file' is being used only in raw_co_delete_file >> at this moment, but it will be used inside LUKS in a later patch. >> Foreseeing this future use, let's put it in block.c and make it >> public. >> >> Suggested-by: Daniel P. Berrangé <berrange@redhat.com> >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> --- >> block.c | 11 +++++++++++ >> block/file-posix.c | 28 ++++++++++++++++++++++++++++ >> include/block/block.h | 1 + >> include/block/block_int.h | 6 ++++++ >> 4 files changed, 46 insertions(+) >> >> diff --git a/block.c b/block.c >> index 0a93ee9ac8..227362b282 100644 >> --- a/block.c >> +++ b/block.c >> @@ -621,6 +621,17 @@ int get_tmp_filename(char *filename, int size) >> #endif >> } >> >> +/** >> + * Helper that checks if a given string represents a regular >> + * local file. >> + */ >> +bool bdrv_path_is_regular_file(const char *path) >> +{ >> + struct stat st; >> + >> + return (stat(path, &st) == 0) && S_ISREG(st.st_mode); >> +} >> + >> /* >> * Detect host devices. By convention, /dev/cdrom[N] is always >> * recognized as a host CDROM. >> diff --git a/block/file-posix.c b/block/file-posix.c >> index d102f3b222..09d84bab37 100644 >> --- a/block/file-posix.c >> +++ b/block/file-posix.c >> @@ -2342,6 +2342,33 @@ static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts, >> return raw_co_create(&options, errp); >> } >> >> +/** >> + * Co-routine function that erases a regular file. >> + */ >> +static int coroutine_fn raw_co_delete_file(const char *filename, >> + Error **errp) > Do we need to mark functions that make no use of coroutines as > coroutine_fn? I guess this way the interface is *allowed* to be a > coroutine if other drivers need to make use of that. This function is used in a coroutine in patch 2. But to be honest, what I did here was to emulate the existing behavior of bdrv_create. Which is kind of lame if bdrv_create happens to have design problems or inconsistencies, but at least it's based on something that's already working. > >> +{ >> + int ret; >> + >> + /* Skip file: protocol prefix */ >> + strstart(filename, "file:", &filename); >> + > This sticks out as fragile to me, but I guess that's exactly how create > works, so... OK. Yep, create does the same thing. Thanks, DHB > >> + if (!bdrv_path_is_regular_file(filename)) { >> + ret = -ENOENT; >> + error_setg_errno(errp, -ret, "%s is not a regular file", filename); >> + goto done; >> + } >> + >> + ret = unlink(filename); >> + if (ret < 0) { >> + ret = -errno; >> + error_setg_errno(errp, -ret, "Error when deleting file %s", filename); >> + } >> + >> +done: >> + return ret; >> +} >> + >> /* >> * Find allocation range in @bs around offset @start. >> * May change underlying file descriptor's file offset. >> @@ -2867,6 +2894,7 @@ BlockDriver bdrv_file = { >> .bdrv_co_block_status = raw_co_block_status, >> .bdrv_co_invalidate_cache = raw_co_invalidate_cache, >> .bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes, >> + .bdrv_co_delete_file = raw_co_delete_file, >> >> .bdrv_co_preadv = raw_co_preadv, >> .bdrv_co_pwritev = raw_co_pwritev, >> diff --git a/include/block/block.h b/include/block/block.h >> index e452988b66..820643f96d 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -363,6 +363,7 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base, >> Error **errp); >> void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base); >> >> +bool bdrv_path_is_regular_file(const char *path); >> >> typedef struct BdrvCheckResult { >> int corruptions; >> diff --git a/include/block/block_int.h b/include/block/block_int.h >> index 01e855a066..74abb78ce7 100644 >> --- a/include/block/block_int.h >> +++ b/include/block/block_int.h >> @@ -309,6 +309,12 @@ struct BlockDriver { >> */ >> int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs); >> >> + /* >> + * Delete a local created file. >> + */ >> + int coroutine_fn (*bdrv_co_delete_file)(const char *filename, >> + Error **errp); >> + >> /* >> * Flushes all data that was already written to the OS all the way down to >> * the disk (for example file-posix.c calls fsync()). >> > Seems alright at a glance, if we want this interface. > > Kevin, do we? > > --js
diff --git a/block.c b/block.c index 0a93ee9ac8..227362b282 100644 --- a/block.c +++ b/block.c @@ -621,6 +621,17 @@ int get_tmp_filename(char *filename, int size) #endif } +/** + * Helper that checks if a given string represents a regular + * local file. + */ +bool bdrv_path_is_regular_file(const char *path) +{ + struct stat st; + + return (stat(path, &st) == 0) && S_ISREG(st.st_mode); +} + /* * Detect host devices. By convention, /dev/cdrom[N] is always * recognized as a host CDROM. diff --git a/block/file-posix.c b/block/file-posix.c index d102f3b222..09d84bab37 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -2342,6 +2342,33 @@ static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts, return raw_co_create(&options, errp); } +/** + * Co-routine function that erases a regular file. + */ +static int coroutine_fn raw_co_delete_file(const char *filename, + Error **errp) +{ + int ret; + + /* Skip file: protocol prefix */ + strstart(filename, "file:", &filename); + + if (!bdrv_path_is_regular_file(filename)) { + ret = -ENOENT; + error_setg_errno(errp, -ret, "%s is not a regular file", filename); + goto done; + } + + ret = unlink(filename); + if (ret < 0) { + ret = -errno; + error_setg_errno(errp, -ret, "Error when deleting file %s", filename); + } + +done: + return ret; +} + /* * Find allocation range in @bs around offset @start. * May change underlying file descriptor's file offset. @@ -2867,6 +2894,7 @@ BlockDriver bdrv_file = { .bdrv_co_block_status = raw_co_block_status, .bdrv_co_invalidate_cache = raw_co_invalidate_cache, .bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes, + .bdrv_co_delete_file = raw_co_delete_file, .bdrv_co_preadv = raw_co_preadv, .bdrv_co_pwritev = raw_co_pwritev, diff --git a/include/block/block.h b/include/block/block.h index e452988b66..820643f96d 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -363,6 +363,7 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base, Error **errp); void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base); +bool bdrv_path_is_regular_file(const char *path); typedef struct BdrvCheckResult { int corruptions; diff --git a/include/block/block_int.h b/include/block/block_int.h index 01e855a066..74abb78ce7 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -309,6 +309,12 @@ struct BlockDriver { */ int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs); + /* + * Delete a local created file. + */ + int coroutine_fn (*bdrv_co_delete_file)(const char *filename, + Error **errp); + /* * Flushes all data that was already written to the OS all the way down to * the disk (for example file-posix.c calls fsync()).
Adding to Block Drivers the capability of being able to clean up its created files can be useful in certain situations. For the LUKS driver, for instance, a failure in one of its authentication steps can leave files in the host that weren't there before. This patch adds the 'bdrv_co_delete_file' interface to block drivers and add it to the 'file' driver in file-posix.c. The implementation is given by 'raw_co_delete_file'. The helper 'bdrv_path_is_regular_file' is being used only in raw_co_delete_file at this moment, but it will be used inside LUKS in a later patch. Foreseeing this future use, let's put it in block.c and make it public. Suggested-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- block.c | 11 +++++++++++ block/file-posix.c | 28 ++++++++++++++++++++++++++++ include/block/block.h | 1 + include/block/block_int.h | 6 ++++++ 4 files changed, 46 insertions(+)