Re: [PATCH v11 04/13] copy-on-read: pass overlay base node name to COR driver
On 14.10.20 18:08, Andrey Shinkevich wrote:
> On 14.10.2020 14:09, Max Reitz wrote:
>> On 12.10.20 19:43, Andrey Shinkevich wrote:
>>> We are going to use the COR-filter for a block-stream job.
>>> To limit COR operations by the base node in the backing chain during
>>> stream job, pass the name of overlay base node to the copy-on-read
>>> driver as base node itself may change due to possible concurrent jobs.
>>> The rest of the functionality will be implemented in the patch that
>>> follows.
>>>
>>> Signed-off-by: Andrey Shinkevich
>>> ---
>>> block/copy-on-read.c | 14 ++
>>> 1 file changed, 14 insertions(+)
>>
>> Is there a reason why you didn’t add this option to QAPI (as part of a
>> yet-to-be-created BlockdevOptionsCor)? Because I’d really like it there.
>>
>
> I agree that passing a base overlay under the base option looks clumsy.
> We could pass the base node name and find its overlay ourselves here in
> cor_open(). In that case, we can use the existing QAPI.
Sounds more complicated than to just split off BlockdevOptionsCor (from
BlockdevOptionsGenericFormat, like e.g. BlockdevOptionsRaw does it).
> The reason I used the existing QAPI is to make it easier for a user to
> operate with the traditional options and to keep things simple. So, the
> user shouldn't think what overlay or above-base node to pass.
Well, they don’t have to pass anything if they don’t need a bottom node.
So unless they want to use a bottom/base-overlay node, it won’t become
more complicated.
> If we introduce the specific BlockdevOptionsCor, what other options may
> come with?
Nothing yet, I think.
>>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
>>> index bcccf0f..c578b1b 100644
>>> --- a/block/copy-on-read.c
>>> +++ b/block/copy-on-read.c
>>> @@ -24,19 +24,24 @@
>>> #include "block/block_int.h"
>>> #include "qemu/module.h"
>>> #include "qapi/error.h"
>>> +#include "qapi/qmp/qerror.h"
>>> #include "qapi/qmp/qdict.h"
>>> #include "block/copy-on-read.h"
>>> typedef struct BDRVStateCOR {
>>> bool active;
>>> + BlockDriverState *base_overlay;
>>> } BDRVStateCOR;
>>> static int cor_open(BlockDriverState *bs, QDict *options, int
>>> flags,
>>> Error **errp)
>>> {
>>> + BlockDriverState *base_overlay = NULL;
>>> BDRVStateCOR *state = bs->opaque;
>>> + /* We need the base overlay node rather than the base itself */
>>> + const char *base_overlay_node = qdict_get_try_str(options, "base");
>>
>> Shouldn’t it be called base-overlay or above-base then?
>>
>
> The base_overlay identifier is used below as the pointer to BS. The
> base_overlay_node stands for the name of the node. I used that
> identifier to differ between the types. And the above_base has another
> meaning per block/stream.c - it can be a temporary filter with a JSON-name.
Yes, agreed; I’m not talking about the variable identifier though. I’m
talking about the option name, which in this version is just “base”.
(And whenever that option is used, once here, once later in
block/stream.c, there is an explanatory comment why we don’t pass the
base node through it, but the first overlay above the base. Which makes
me believe perhaps it shouldn’t be called “base”.)
>>> bs->file = bdrv_open_child(NULL, options, "file", bs,
>>> _of_bds,
>>> BDRV_CHILD_FILTERED |
>>> BDRV_CHILD_PRIMARY,
>>> @@ -52,7 +57,16 @@ static int cor_open(BlockDriverState *bs, QDict
>>> *options, int flags,
>>> ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
>>> bs->file->bs->supported_zero_flags);
>>> + if (base_overlay_node) {
>>> + qdict_del(options, "base");
>>> + base_overlay = bdrv_lookup_bs(NULL, base_overlay_node, errp);
>>
>> I think this is a use-after-free. The storage @base_overlay_node points
>> to belongs to a QString, which is referenced only by @options; so
>> deleting that element of @options should free that string.
>>
>> Max
>>
>
> I will swap those two function calls (bdrv_lookup_bs(); qdict_del();).
> Thank you.
Don’t forget that the error_setg() below also makes use of it:
>>> + if (!base_overlay) {
>>> + error_setg(errp, QERR_BASE_NOT_FOUND, base_overlay_node);
>>> + return -EINVAL;
>>> + }
>>> + }
Max
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v11 04/13] copy-on-read: pass overlay base node name to COR driver
On 14.10.2020 14:09, Max Reitz wrote:
On 12.10.20 19:43, Andrey Shinkevich wrote:
We are going to use the COR-filter for a block-stream job.
To limit COR operations by the base node in the backing chain during
stream job, pass the name of overlay base node to the copy-on-read
driver as base node itself may change due to possible concurrent jobs.
The rest of the functionality will be implemented in the patch that
follows.
Signed-off-by: Andrey Shinkevich
---
block/copy-on-read.c | 14 ++
1 file changed, 14 insertions(+)
Is there a reason why you didn’t add this option to QAPI (as part of a
yet-to-be-created BlockdevOptionsCor)? Because I’d really like it there.
I agree that passing a base overlay under the base option looks clumsy.
We could pass the base node name and find its overlay ourselves here in
cor_open(). In that case, we can use the existing QAPI.
The reason I used the existing QAPI is to make it easier for a user to
operate with the traditional options and to keep things simple. So, the
user shouldn't think what overlay or above-base node to pass.
If we introduce the specific BlockdevOptionsCor, what other options may
come with?
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index bcccf0f..c578b1b 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -24,19 +24,24 @@
#include "block/block_int.h"
#include "qemu/module.h"
#include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
#include "qapi/qmp/qdict.h"
#include "block/copy-on-read.h"
typedef struct BDRVStateCOR {
bool active;
+BlockDriverState *base_overlay;
} BDRVStateCOR;
static int cor_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
+BlockDriverState *base_overlay = NULL;
BDRVStateCOR *state = bs->opaque;
+/* We need the base overlay node rather than the base itself */
+const char *base_overlay_node = qdict_get_try_str(options, "base");
Shouldn’t it be called base-overlay or above-base then?
The base_overlay identifier is used below as the pointer to BS. The
base_overlay_node stands for the name of the node. I used that
identifier to differ between the types. And the above_base has another
meaning per block/stream.c - it can be a temporary filter with a JSON-name.
bs->file = bdrv_open_child(NULL, options, "file", bs, _of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
@@ -52,7 +57,16 @@ static int cor_open(BlockDriverState *bs, QDict *options,
int flags,
((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
bs->file->bs->supported_zero_flags);
+if (base_overlay_node) {
+qdict_del(options, "base");
+base_overlay = bdrv_lookup_bs(NULL, base_overlay_node, errp);
I think this is a use-after-free. The storage @base_overlay_node points
to belongs to a QString, which is referenced only by @options; so
deleting that element of @options should free that string.
Max
I will swap those two function calls (bdrv_lookup_bs(); qdict_del();).
Thank you.
Andrey
+if (!base_overlay) {
+error_setg(errp, QERR_BASE_NOT_FOUND, base_overlay_node);
+return -EINVAL;
+}
+}
state->active = true;
+state->base_overlay = base_overlay;
/*
* We don't need to call bdrv_child_refresh_perms() now as the permissions
Re: [PATCH v11 04/13] copy-on-read: pass overlay base node name to COR driver
On 14.10.20 16:56, Vladimir Sementsov-Ogievskiy wrote:
> 14.10.2020 14:57, Max Reitz wrote:
>> On 14.10.20 13:09, Max Reitz wrote:
>>> On 12.10.20 19:43, Andrey Shinkevich wrote:
We are going to use the COR-filter for a block-stream job.
To limit COR operations by the base node in the backing chain during
stream job, pass the name of overlay base node to the copy-on-read
driver as base node itself may change due to possible concurrent jobs.
The rest of the functionality will be implemented in the patch that
follows.
Signed-off-by: Andrey Shinkevich
---
block/copy-on-read.c | 14 ++
1 file changed, 14 insertions(+)
>>>
>>> Is there a reason why you didn’t add this option to QAPI (as part of a
>>> yet-to-be-created BlockdevOptionsCor)? Because I’d really like it
>>> there.
>>>
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index bcccf0f..c578b1b 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -24,19 +24,24 @@
#include "block/block_int.h"
#include "qemu/module.h"
#include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
#include "qapi/qmp/qdict.h"
#include "block/copy-on-read.h"
typedef struct BDRVStateCOR {
bool active;
+ BlockDriverState *base_overlay;
} BDRVStateCOR;
static int cor_open(BlockDriverState *bs, QDict *options, int
flags,
Error **errp)
{
+ BlockDriverState *base_overlay = NULL;
BDRVStateCOR *state = bs->opaque;
+ /* We need the base overlay node rather than the base itself */
+ const char *base_overlay_node = qdict_get_try_str(options,
"base");
>>>
>>> Shouldn’t it be called base-overlay or above-base then?
>>
>> While reviewing patch 5, I realized this sould indeed be base-overlay
(Just realized that sounds different from how I meant it. I meant that
“above-base” would be wrong, so from the two, if any, it should be
“base-overlay”.)
>> (i.e. the next allocation-bearing layer above the base, not just a
>> filter on top of base), but that should be noted somewhere, of course.
>> Best would be the QAPI documentation for this option. O:)
>>
>
> What about naming it just "bottom" or "bottom-node"? If we don't have
> base, it's strange to have something "relative to base". And we can
> document, that "bottom" must be a non-filter node in a backing chain of
> "top".
Works for me, too.
Max
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v11 04/13] copy-on-read: pass overlay base node name to COR driver
14.10.2020 19:08, Andrey Shinkevich wrote:
On 14.10.2020 14:09, Max Reitz wrote:
On 12.10.20 19:43, Andrey Shinkevich wrote:
We are going to use the COR-filter for a block-stream job.
To limit COR operations by the base node in the backing chain during
stream job, pass the name of overlay base node to the copy-on-read
driver as base node itself may change due to possible concurrent jobs.
The rest of the functionality will be implemented in the patch that
follows.
Signed-off-by: Andrey Shinkevich
---
block/copy-on-read.c | 14 ++
1 file changed, 14 insertions(+)
Is there a reason why you didn’t add this option to QAPI (as part of a
yet-to-be-created BlockdevOptionsCor)? Because I’d really like it there.
I agree that passing a base overlay under the base option looks clumsy. We
could pass the base node name and find its overlay ourselves here in
cor_open(). In that case, we can use the existing QAPI.
Actually, there is no existing QAPI: if you don't modify qapi/*.json, user is
not able to pass the option through QAPI. It's still possible to pass the
option through command-line, or when create the filter internally (like we are
going to do in block-stream), but not through QAPI. So, it's better to make a
new QAPI parameter, to make the new option available for QMP interface.
The reason I used the existing QAPI is to make it easier for a user to operate
with the traditional options and to keep things simple. So, the user shouldn't
think what overlay or above-base node to pass.
If we introduce the specific BlockdevOptionsCor, what other options may come
with?
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index bcccf0f..c578b1b 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -24,19 +24,24 @@
#include "block/block_int.h"
#include "qemu/module.h"
#include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
#include "qapi/qmp/qdict.h"
#include "block/copy-on-read.h"
typedef struct BDRVStateCOR {
bool active;
+ BlockDriverState *base_overlay;
} BDRVStateCOR;
static int cor_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
+ BlockDriverState *base_overlay = NULL;
BDRVStateCOR *state = bs->opaque;
+ /* We need the base overlay node rather than the base itself */
+ const char *base_overlay_node = qdict_get_try_str(options, "base");
Shouldn’t it be called base-overlay or above-base then?
The base_overlay identifier is used below as the pointer to BS. The
base_overlay_node stands for the name of the node. I used that identifier to
differ between the types. And the above_base has another meaning per
block/stream.c - it can be a temporary filter with a JSON-name.
bs->file = bdrv_open_child(NULL, options, "file", bs, _of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
@@ -52,7 +57,16 @@ static int cor_open(BlockDriverState *bs, QDict *options,
int flags,
((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
bs->file->bs->supported_zero_flags);
+ if (base_overlay_node) {
+ qdict_del(options, "base");
+ base_overlay = bdrv_lookup_bs(NULL, base_overlay_node, errp);
I think this is a use-after-free. The storage @base_overlay_node points
to belongs to a QString, which is referenced only by @options; so
deleting that element of @options should free that string.
Max
I will swap those two function calls (bdrv_lookup_bs(); qdict_del();).
Thank you.
Andrey
+ if (!base_overlay) {
+ error_setg(errp, QERR_BASE_NOT_FOUND, base_overlay_node);
+ return -EINVAL;
+ }
+ }
state->active = true;
+ state->base_overlay = base_overlay;
/*
* We don't need to call bdrv_child_refresh_perms() now as the permissions
--
Best regards,
Vladimir
Re: [PATCH v11 04/13] copy-on-read: pass overlay base node name to COR driver
14.10.2020 14:57, Max Reitz wrote:
On 14.10.20 13:09, Max Reitz wrote:
On 12.10.20 19:43, Andrey Shinkevich wrote:
We are going to use the COR-filter for a block-stream job.
To limit COR operations by the base node in the backing chain during
stream job, pass the name of overlay base node to the copy-on-read
driver as base node itself may change due to possible concurrent jobs.
The rest of the functionality will be implemented in the patch that
follows.
Signed-off-by: Andrey Shinkevich
---
block/copy-on-read.c | 14 ++
1 file changed, 14 insertions(+)
Is there a reason why you didn’t add this option to QAPI (as part of a
yet-to-be-created BlockdevOptionsCor)? Because I’d really like it there.
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index bcccf0f..c578b1b 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -24,19 +24,24 @@
#include "block/block_int.h"
#include "qemu/module.h"
#include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
#include "qapi/qmp/qdict.h"
#include "block/copy-on-read.h"
typedef struct BDRVStateCOR {
bool active;
+BlockDriverState *base_overlay;
} BDRVStateCOR;
static int cor_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
+BlockDriverState *base_overlay = NULL;
BDRVStateCOR *state = bs->opaque;
+/* We need the base overlay node rather than the base itself */
+const char *base_overlay_node = qdict_get_try_str(options, "base");
Shouldn’t it be called base-overlay or above-base then?
While reviewing patch 5, I realized this sould indeed be base-overlay
(i.e. the next allocation-bearing layer above the base, not just a
filter on top of base), but that should be noted somewhere, of course.
Best would be the QAPI documentation for this option. O:)
What about naming it just "bottom" or "bottom-node"? If we don't have base, it's strange to have something
"relative to base". And we can document, that "bottom" must be a non-filter node in a backing chain of
"top".
--
Best regards,
Vladimir
Re: [PATCH v11 04/13] copy-on-read: pass overlay base node name to COR driver
On 14.10.20 13:09, Max Reitz wrote:
> On 12.10.20 19:43, Andrey Shinkevich wrote:
>> We are going to use the COR-filter for a block-stream job.
>> To limit COR operations by the base node in the backing chain during
>> stream job, pass the name of overlay base node to the copy-on-read
>> driver as base node itself may change due to possible concurrent jobs.
>> The rest of the functionality will be implemented in the patch that
>> follows.
>>
>> Signed-off-by: Andrey Shinkevich
>> ---
>> block/copy-on-read.c | 14 ++
>> 1 file changed, 14 insertions(+)
>
> Is there a reason why you didn’t add this option to QAPI (as part of a
> yet-to-be-created BlockdevOptionsCor)? Because I’d really like it there.
>
>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
>> index bcccf0f..c578b1b 100644
>> --- a/block/copy-on-read.c
>> +++ b/block/copy-on-read.c
>> @@ -24,19 +24,24 @@
>> #include "block/block_int.h"
>> #include "qemu/module.h"
>> #include "qapi/error.h"
>> +#include "qapi/qmp/qerror.h"
>> #include "qapi/qmp/qdict.h"
>> #include "block/copy-on-read.h"
>>
>>
>> typedef struct BDRVStateCOR {
>> bool active;
>> +BlockDriverState *base_overlay;
>> } BDRVStateCOR;
>>
>>
>> static int cor_open(BlockDriverState *bs, QDict *options, int flags,
>> Error **errp)
>> {
>> +BlockDriverState *base_overlay = NULL;
>> BDRVStateCOR *state = bs->opaque;
>> +/* We need the base overlay node rather than the base itself */
>> +const char *base_overlay_node = qdict_get_try_str(options, "base");
>
> Shouldn’t it be called base-overlay or above-base then?
While reviewing patch 5, I realized this sould indeed be base-overlay
(i.e. the next allocation-bearing layer above the base, not just a
filter on top of base), but that should be noted somewhere, of course.
Best would be the QAPI documentation for this option. O:)
Max
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v11 04/13] copy-on-read: pass overlay base node name to COR driver
On 12.10.20 19:43, Andrey Shinkevich wrote:
> We are going to use the COR-filter for a block-stream job.
> To limit COR operations by the base node in the backing chain during
> stream job, pass the name of overlay base node to the copy-on-read
> driver as base node itself may change due to possible concurrent jobs.
> The rest of the functionality will be implemented in the patch that
> follows.
>
> Signed-off-by: Andrey Shinkevich
> ---
> block/copy-on-read.c | 14 ++
> 1 file changed, 14 insertions(+)
Is there a reason why you didn’t add this option to QAPI (as part of a
yet-to-be-created BlockdevOptionsCor)? Because I’d really like it there.
> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> index bcccf0f..c578b1b 100644
> --- a/block/copy-on-read.c
> +++ b/block/copy-on-read.c
> @@ -24,19 +24,24 @@
> #include "block/block_int.h"
> #include "qemu/module.h"
> #include "qapi/error.h"
> +#include "qapi/qmp/qerror.h"
> #include "qapi/qmp/qdict.h"
> #include "block/copy-on-read.h"
>
>
> typedef struct BDRVStateCOR {
> bool active;
> +BlockDriverState *base_overlay;
> } BDRVStateCOR;
>
>
> static int cor_open(BlockDriverState *bs, QDict *options, int flags,
> Error **errp)
> {
> +BlockDriverState *base_overlay = NULL;
> BDRVStateCOR *state = bs->opaque;
> +/* We need the base overlay node rather than the base itself */
> +const char *base_overlay_node = qdict_get_try_str(options, "base");
Shouldn’t it be called base-overlay or above-base then?
>
> bs->file = bdrv_open_child(NULL, options, "file", bs, _of_bds,
> BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
> @@ -52,7 +57,16 @@ static int cor_open(BlockDriverState *bs, QDict *options,
> int flags,
> ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
> bs->file->bs->supported_zero_flags);
>
> +if (base_overlay_node) {
> +qdict_del(options, "base");
> +base_overlay = bdrv_lookup_bs(NULL, base_overlay_node, errp);
I think this is a use-after-free. The storage @base_overlay_node points
to belongs to a QString, which is referenced only by @options; so
deleting that element of @options should free that string.
Max
> +if (!base_overlay) {
> +error_setg(errp, QERR_BASE_NOT_FOUND, base_overlay_node);
> +return -EINVAL;
> +}
> +}
> state->active = true;
> +state->base_overlay = base_overlay;
>
> /*
> * We don't need to call bdrv_child_refresh_perms() now as the
> permissions
>
signature.asc
Description: OpenPGP digital signature
[PATCH v11 04/13] copy-on-read: pass overlay base node name to COR driver
We are going to use the COR-filter for a block-stream job.
To limit COR operations by the base node in the backing chain during
stream job, pass the name of overlay base node to the copy-on-read
driver as base node itself may change due to possible concurrent jobs.
The rest of the functionality will be implemented in the patch that
follows.
Signed-off-by: Andrey Shinkevich
---
block/copy-on-read.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index bcccf0f..c578b1b 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -24,19 +24,24 @@
#include "block/block_int.h"
#include "qemu/module.h"
#include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
#include "qapi/qmp/qdict.h"
#include "block/copy-on-read.h"
typedef struct BDRVStateCOR {
bool active;
+BlockDriverState *base_overlay;
} BDRVStateCOR;
static int cor_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
+BlockDriverState *base_overlay = NULL;
BDRVStateCOR *state = bs->opaque;
+/* We need the base overlay node rather than the base itself */
+const char *base_overlay_node = qdict_get_try_str(options, "base");
bs->file = bdrv_open_child(NULL, options, "file", bs, _of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
@@ -52,7 +57,16 @@ static int cor_open(BlockDriverState *bs, QDict *options,
int flags,
((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
bs->file->bs->supported_zero_flags);
+if (base_overlay_node) {
+qdict_del(options, "base");
+base_overlay = bdrv_lookup_bs(NULL, base_overlay_node, errp);
+if (!base_overlay) {
+error_setg(errp, QERR_BASE_NOT_FOUND, base_overlay_node);
+return -EINVAL;
+}
+}
state->active = true;
+state->base_overlay = base_overlay;
/*
* We don't need to call bdrv_child_refresh_perms() now as the permissions
--
1.8.3.1
