On Tue, May 28, 2013 at 02:44:26PM +0800, Fam Zheng wrote:
> We may want to include a driver in the whitelist for read only tasks
> such as diagnosing or exporting guest data (with libguestfs as a good
> example). This patch introduces the magic prefix ^ to include a driver
> to the whitelist, but only enables qemu to open specific image
> format readonly, and returns -ENOTSUP for RW opening.
>
> Example: To include vmdk readonly, and others read+write:
> ./configure --block-drv-whitelist=qcow2,raw,file,qed,^vmdk
This is great, thanks for tackling this. block/vmdk.c isn't suitable
for running real VMs (read-write) since it's not optimized for
concurrent I/O but it is usable for libguestfs (read-only).
> Signed-off-by: Fam Zheng <f...@redhat.com>
> ---
> block.c | 43 +++++++++++++++++++++++++++----------------
> blockdev.c | 4 ++--
> configure | 2 ++
> include/block/block.h | 3 ++-
> scripts/create_config | 9 ++++++++-
> 5 files changed, 41 insertions(+), 20 deletions(-)
>
> diff --git a/block.c b/block.c
> index 3f87489..e6a7270 100644
> --- a/block.c
> +++ b/block.c
> @@ -328,28 +328,40 @@ BlockDriver *bdrv_find_format(const char *format_name)
> return NULL;
> }
>
> -static int bdrv_is_whitelisted(BlockDriver *drv)
> +static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
> {
> - static const char *whitelist[] = {
> - CONFIG_BDRV_WHITELIST
> + static const char *whitelist_rw[] = {
> + CONFIG_BDRV_WHITELIST_RW
> + };
> + static const char *whitelist_ro[] = {
> + CONFIG_BDRV_WHITELIST_RO
> };
> const char **p;
Please also make the ./configure lists separate. The funky ^ syntax is
not obvious. Better to have:
./configure --block-drv-whitelist-rw=qcow2,raw,file,qed \
--block-drv-whitelist-ro=vmdk,vpc
>
> - if (!whitelist[0])
> + if (!whitelist_rw[0] && !whitelist_ro[0]) {
> return 1; /* no whitelist, anything goes */
> + }
>
> - for (p = whitelist; *p; p++) {
> + for (p = whitelist_rw; *p; p++) {
> if (!strcmp(drv->format_name, *p)) {
> return 1;
> }
> }
> + if (read_only) {
> + for (p = whitelist_ro; *p; p++) {
> + if (!strcmp(drv->format_name, *p)) {
> + return 1;
> + }
> + }
> + }
> return 0;
> }
>
> -BlockDriver *bdrv_find_whitelisted_format(const char *format_name)
> +BlockDriver *bdrv_find_whitelisted_format(const char *format_name,
> + bool read_only)
> {
> BlockDriver *drv = bdrv_find_format(format_name);
> - return drv && bdrv_is_whitelisted(drv) ? drv : NULL;
> + return drv && bdrv_is_whitelisted(drv, read_only) ? drv : NULL;
> }
>
> typedef struct CreateCo {
> @@ -684,10 +696,6 @@ static int bdrv_open_common(BlockDriverState *bs,
> BlockDriverState *file,
>
> trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name);
>
> - if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) {
> - return -ENOTSUP;
> - }
> -
> /* bdrv_open() with directly using a protocol as drv. This layer is
> already
> * opened, so assign it to bs (while file becomes a closed
> BlockDriverState)
> * and return immediately. */
if (file != NULL && drv->bdrv_file_open) {
bdrv_swap(file, bs);
return 0;
}
I think your change is okay. You moved the check after this early
return, but file is already opened so we passed the whitelist check
already. This is a little tricky but it seems fine.
Stefan
