[v3,3/5] block/null: Implement bdrv_refresh_filename()
diff mbox

Message ID 20160610185750.30956-4-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz June 10, 2016, 6:57 p.m. UTC
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/null.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Fam Zheng June 12, 2016, 4:08 a.m. UTC | #1
On Fri, 06/10 20:57, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>

The commit message could go a little more informative. Seems nothing special in
the added callback, aren't things supposed to just work without this patch?
What is missing?

> ---
>  block/null.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/block/null.c b/block/null.c
> index 396500b..b511010 100644
> --- a/block/null.c
> +++ b/block/null.c
> @@ -12,6 +12,8 @@
>  
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qstring.h"
>  #include "block/block_int.h"
>  
>  #define NULL_OPT_LATENCY "latency-ns"
> @@ -223,6 +225,20 @@ static int64_t coroutine_fn null_co_get_block_status(BlockDriverState *bs,
>      }
>  }
>  
> +static void null_refresh_filename(BlockDriverState *bs, QDict *opts)
> +{
> +    QINCREF(opts);
> +    qdict_del(opts, "filename");

Why is this qdict_del necessary?

> +
> +    if (!qdict_size(opts)) {
> +        snprintf(bs->exact_filename, sizeof(bs->exact_filename), "%s://",
> +                 bs->drv->format_name);
> +    }
> +
> +    qdict_put(opts, "driver", qstring_from_str(bs->drv->format_name));
> +    bs->full_open_options = opts;
> +}
> +
>  static BlockDriver bdrv_null_co = {
>      .format_name            = "null-co",
>      .protocol_name          = "null-co",
> @@ -238,6 +254,8 @@ static BlockDriver bdrv_null_co = {
>      .bdrv_reopen_prepare    = null_reopen_prepare,
>  
>      .bdrv_co_get_block_status   = null_co_get_block_status,
> +
> +    .bdrv_refresh_filename  = null_refresh_filename,
>  };
>  
>  static BlockDriver bdrv_null_aio = {
> @@ -255,6 +273,8 @@ static BlockDriver bdrv_null_aio = {
>      .bdrv_reopen_prepare    = null_reopen_prepare,
>  
>      .bdrv_co_get_block_status   = null_co_get_block_status,
> +
> +    .bdrv_refresh_filename  = null_refresh_filename,
>  };
>  
>  static void bdrv_null_init(void)
> -- 
> 2.8.3
>
Max Reitz June 14, 2016, 12:59 p.m. UTC | #2
On 12.06.2016 06:08, Fam Zheng wrote:
> On Fri, 06/10 20:57, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> The commit message could go a little more informative. Seems nothing special in
> the added callback, aren't things supposed to just work without this patch?
> What is missing?

If you pass a filename, it works without this patch. If you don't, it
doesn't.

Compare (before this patch):

$ x86_64-softmmu/qemu-system-x86_64 \
  -drive if=none,file=null-co://,driver=raw -nodefaults -qmp stdio
[...]
{'execute':'query-block'}
[...] "filename": "null-co://", [...]

With:

$ x86_64-softmmu/qemu-system-x86_64 \
  -drive if=none,driver=raw,file.driver=null-co -nodefaults -qmp stdio
[...]
{'execute':'query-block'}
[...] "filename": "json:{\"driver\": \"raw\", \"file\": {\"driver\":
\"null-co\"}}", [...]


So the issue is that you can use the null-co/aio drivers without giving
a filename.

I wouldn't mind including this information in a v4, but I'm not sure it
alone warrants a v4.

> 
>> ---
>>  block/null.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/block/null.c b/block/null.c
>> index 396500b..b511010 100644
>> --- a/block/null.c
>> +++ b/block/null.c
>> @@ -12,6 +12,8 @@
>>  
>>  #include "qemu/osdep.h"
>>  #include "qapi/error.h"
>> +#include "qapi/qmp/qdict.h"
>> +#include "qapi/qmp/qstring.h"
>>  #include "block/block_int.h"
>>  
>>  #define NULL_OPT_LATENCY "latency-ns"
>> @@ -223,6 +225,20 @@ static int64_t coroutine_fn null_co_get_block_status(BlockDriverState *bs,
>>      }
>>  }
>>  
>> +static void null_refresh_filename(BlockDriverState *bs, QDict *opts)
>> +{
>> +    QINCREF(opts);
>> +    qdict_del(opts, "filename");
> 
> Why is this qdict_del necessary?

It's not strictly necessary. But the null drivers ignore the filename,
so there's no harm in dropping it from the JSON filename (if we need to
construct one).

Max

>> +
>> +    if (!qdict_size(opts)) {
>> +        snprintf(bs->exact_filename, sizeof(bs->exact_filename), "%s://",
>> +                 bs->drv->format_name);
>> +    }
>> +
>> +    qdict_put(opts, "driver", qstring_from_str(bs->drv->format_name));
>> +    bs->full_open_options = opts;
>> +}
>> +
>>  static BlockDriver bdrv_null_co = {
>>      .format_name            = "null-co",
>>      .protocol_name          = "null-co",
>> @@ -238,6 +254,8 @@ static BlockDriver bdrv_null_co = {
>>      .bdrv_reopen_prepare    = null_reopen_prepare,
>>  
>>      .bdrv_co_get_block_status   = null_co_get_block_status,
>> +
>> +    .bdrv_refresh_filename  = null_refresh_filename,
>>  };
>>  
>>  static BlockDriver bdrv_null_aio = {
>> @@ -255,6 +273,8 @@ static BlockDriver bdrv_null_aio = {
>>      .bdrv_reopen_prepare    = null_reopen_prepare,
>>  
>>      .bdrv_co_get_block_status   = null_co_get_block_status,
>> +
>> +    .bdrv_refresh_filename  = null_refresh_filename,
>>  };
>>  
>>  static void bdrv_null_init(void)
>> -- 
>> 2.8.3
>>

Patch
diff mbox

diff --git a/block/null.c b/block/null.c
index 396500b..b511010 100644
--- a/block/null.c
+++ b/block/null.c
@@ -12,6 +12,8 @@ 
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qstring.h"
 #include "block/block_int.h"
 
 #define NULL_OPT_LATENCY "latency-ns"
@@ -223,6 +225,20 @@  static int64_t coroutine_fn null_co_get_block_status(BlockDriverState *bs,
     }
 }
 
+static void null_refresh_filename(BlockDriverState *bs, QDict *opts)
+{
+    QINCREF(opts);
+    qdict_del(opts, "filename");
+
+    if (!qdict_size(opts)) {
+        snprintf(bs->exact_filename, sizeof(bs->exact_filename), "%s://",
+                 bs->drv->format_name);
+    }
+
+    qdict_put(opts, "driver", qstring_from_str(bs->drv->format_name));
+    bs->full_open_options = opts;
+}
+
 static BlockDriver bdrv_null_co = {
     .format_name            = "null-co",
     .protocol_name          = "null-co",
@@ -238,6 +254,8 @@  static BlockDriver bdrv_null_co = {
     .bdrv_reopen_prepare    = null_reopen_prepare,
 
     .bdrv_co_get_block_status   = null_co_get_block_status,
+
+    .bdrv_refresh_filename  = null_refresh_filename,
 };
 
 static BlockDriver bdrv_null_aio = {
@@ -255,6 +273,8 @@  static BlockDriver bdrv_null_aio = {
     .bdrv_reopen_prepare    = null_reopen_prepare,
 
     .bdrv_co_get_block_status   = null_co_get_block_status,
+
+    .bdrv_refresh_filename  = null_refresh_filename,
 };
 
 static void bdrv_null_init(void)