Re: [PATCH v12 13/14] stream: skip filters when writing backing file name to QCOW2 header

2020-10-27 Thread Vladimir Sementsov-Ogievskiy

27.10.2020 19:42, Andrey Shinkevich wrote:

On 27.10.2020 19:21, Vladimir Sementsov-Ogievskiy wrote:

27.10.2020 19:01, Andrey Shinkevich wrote:

On 27.10.2020 18:09, Vladimir Sementsov-Ogievskiy wrote:

22.10.2020 21:13, Andrey Shinkevich wrote:

Avoid writing a filter JSON file name and a filter format name to QCOW2
image when the backing file is changed after the block stream job.
A user is still able to assign the 'backing-file' parameter for a
block-stream job keeping in mind the possible issue mentioned above.
If the user does not specify the 'backing-file' parameter, QEMU will
assign it automatically.

Signed-off-by: Andrey Shinkevich 
---
  block/stream.c | 15 +--
  blockdev.c |  9 ++---
  2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index e0540ee..1ba74ab 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -65,6 +65,7 @@ static int stream_prepare(Job *job)
  BlockDriverState *bs = blk_bs(bjob->blk);
  BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
  BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
+    BlockDriverState *base_unfiltered = NULL;
  Error *local_err = NULL;
  int ret = 0;
@@ -75,8 +76,18 @@ static int stream_prepare(Job *job)
  const char *base_id = NULL, *base_fmt = NULL;
  if (base) {
  base_id = s->backing_file_str;
-    if (base->drv) {
-    base_fmt = base->drv->format_name;
+    if (base_id) {
+    if (base->drv) {
+    base_fmt = base->drv->format_name;


hmm. this doesn't make real sense: so, we assume that user specified 
backing_file_str, which may not relate to base, but we use 
base->drv->format_name? But it may be name of the filter driver, which would be 
wrong..

Any ideas?

1. we can use base_fmt=NULL, to provoke probing on next open of the qcow2 file..


I would choose this item #1 but have to check the probing code logic... 
Particularly, I do not remember now if the probing is able to recognize a 
protocol.
The logic for the format_name in the QEMU existent code (I has kept it here in 
the patch) is a slippery way for an imprudent user. That's why I staked on the 
backing_file_str deprication in the previous version.


2. we can do probing now
3. we can at least check, if backing_file_str == 


Not bad for the sanity check but we will search a node by the file name again - 
not good ((


Not search, but only check one very likely option.


Yes, just strcmp(). And why a user may not merely specify a desired backing 
file as the base?


*shrung*





Additionally to 1. or 3. (or combined), or even keeping things as is (i.e. 
wrong, but it is preexisting), we can:

  - add backing-format argument to qapi as pair for backing-file
  - deprecate using backing-file without backing-format.

Then, after deprecation period we'll have correct code. This may be done in 
separate.




base_unfiltered->filename, in this case we can use 
base_unfiltered->drv->format_name



+    }
+    } else {
+    base_unfiltered = bdrv_skip_filters(base);
+    if (base_unfiltered) {
+    base_id = base_unfiltered->filename;
+    if (base_unfiltered->drv) {
+    base_fmt = base_unfiltered->drv->format_name;
+    }
+    }
  }
  }
  bdrv_set_backing_hd(unfiltered_bs, base, _err);
diff --git a/blockdev.c b/blockdev.c
index c917625..0e9c783 100644
--- a/blockdev.c
+++ b/blockdev.c


[...]


-    stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
+    stream_start(has_job_id ? job_id : NULL, bs, base_bs,
+ has_backing_file ? backing_file : NULL,


backing_file should be NULL if has_backing_file is false, so you can use just 
backing_file instead of ternary operator.



Yes, if reliable. I has kept the conformation with the ternary operator at the 
first parameter above.

Andrey


   job_flags, has_speed ? speed : 0, on_error,
   filter_node_name, _err);
  if (local_err) {










--
Best regards,
Vladimir



Re: [PATCH v12 13/14] stream: skip filters when writing backing file name to QCOW2 header

2020-10-27 Thread Andrey Shinkevich

On 27.10.2020 19:21, Vladimir Sementsov-Ogievskiy wrote:

27.10.2020 19:01, Andrey Shinkevich wrote:

On 27.10.2020 18:09, Vladimir Sementsov-Ogievskiy wrote:

22.10.2020 21:13, Andrey Shinkevich wrote:

Avoid writing a filter JSON file name and a filter format name to QCOW2
image when the backing file is changed after the block stream job.
A user is still able to assign the 'backing-file' parameter for a
block-stream job keeping in mind the possible issue mentioned above.
If the user does not specify the 'backing-file' parameter, QEMU will
assign it automatically.

Signed-off-by: Andrey Shinkevich 
---
  block/stream.c | 15 +--
  blockdev.c |  9 ++---
  2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index e0540ee..1ba74ab 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -65,6 +65,7 @@ static int stream_prepare(Job *job)
  BlockDriverState *bs = blk_bs(bjob->blk);
  BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
  BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
+    BlockDriverState *base_unfiltered = NULL;
  Error *local_err = NULL;
  int ret = 0;
@@ -75,8 +76,18 @@ static int stream_prepare(Job *job)
  const char *base_id = NULL, *base_fmt = NULL;
  if (base) {
  base_id = s->backing_file_str;
-    if (base->drv) {
-    base_fmt = base->drv->format_name;
+    if (base_id) {
+    if (base->drv) {
+    base_fmt = base->drv->format_name;


hmm. this doesn't make real sense: so, we assume that user specified 
backing_file_str, which may not relate to base, but we use 
base->drv->format_name? But it may be name of the filter driver, 
which would be wrong..


Any ideas?

1. we can use base_fmt=NULL, to provoke probing on next open of the 
qcow2 file..


I would choose this item #1 but have to check the probing code 
logic... Particularly, I do not remember now if the probing is able to 
recognize a protocol.
The logic for the format_name in the QEMU existent code (I has kept it 
here in the patch) is a slippery way for an imprudent user. That's why 
I staked on the backing_file_str deprication in the previous version.



2. we can do probing now
3. we can at least check, if backing_file_str == 


Not bad for the sanity check but we will search a node by the file 
name again - not good ((


Not search, but only check one very likely option.


Yes, just strcmp(). And why a user may not merely specify a desired 
backing file as the base?




Additionally to 1. or 3. (or combined), or even keeping things as is 
(i.e. wrong, but it is preexisting), we can:


  - add backing-format argument to qapi as pair for backing-file
  - deprecate using backing-file without backing-format.

Then, after deprecation period we'll have correct code. This may be done 
in separate.




base_unfiltered->filename, in this case we can use 
base_unfiltered->drv->format_name




+    }
+    } else {
+    base_unfiltered = bdrv_skip_filters(base);
+    if (base_unfiltered) {
+    base_id = base_unfiltered->filename;
+    if (base_unfiltered->drv) {
+    base_fmt = base_unfiltered->drv->format_name;
+    }
+    }
  }
  }
  bdrv_set_backing_hd(unfiltered_bs, base, _err);
diff --git a/blockdev.c b/blockdev.c
index c917625..0e9c783 100644
--- a/blockdev.c
+++ b/blockdev.c


[...]


-    stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
+    stream_start(has_job_id ? job_id : NULL, bs, base_bs,
+ has_backing_file ? backing_file : NULL,


backing_file should be NULL if has_backing_file is false, so you can 
use just backing_file instead of ternary operator.




Yes, if reliable. I has kept the conformation with the ternary 
operator at the first parameter above.


Andrey


   job_flags, has_speed ? speed : 0, on_error,
   filter_node_name, _err);
  if (local_err) {











Re: [PATCH v12 13/14] stream: skip filters when writing backing file name to QCOW2 header

2020-10-27 Thread Vladimir Sementsov-Ogievskiy

27.10.2020 19:01, Andrey Shinkevich wrote:

On 27.10.2020 18:09, Vladimir Sementsov-Ogievskiy wrote:

22.10.2020 21:13, Andrey Shinkevich wrote:

Avoid writing a filter JSON file name and a filter format name to QCOW2
image when the backing file is changed after the block stream job.
A user is still able to assign the 'backing-file' parameter for a
block-stream job keeping in mind the possible issue mentioned above.
If the user does not specify the 'backing-file' parameter, QEMU will
assign it automatically.

Signed-off-by: Andrey Shinkevich 
---
  block/stream.c | 15 +--
  blockdev.c |  9 ++---
  2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index e0540ee..1ba74ab 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -65,6 +65,7 @@ static int stream_prepare(Job *job)
  BlockDriverState *bs = blk_bs(bjob->blk);
  BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
  BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
+    BlockDriverState *base_unfiltered = NULL;
  Error *local_err = NULL;
  int ret = 0;
@@ -75,8 +76,18 @@ static int stream_prepare(Job *job)
  const char *base_id = NULL, *base_fmt = NULL;
  if (base) {
  base_id = s->backing_file_str;
-    if (base->drv) {
-    base_fmt = base->drv->format_name;
+    if (base_id) {
+    if (base->drv) {
+    base_fmt = base->drv->format_name;


hmm. this doesn't make real sense: so, we assume that user specified 
backing_file_str, which may not relate to base, but we use 
base->drv->format_name? But it may be name of the filter driver, which would be 
wrong..

Any ideas?

1. we can use base_fmt=NULL, to provoke probing on next open of the qcow2 file..


I would choose this item #1 but have to check the probing code logic... 
Particularly, I do not remember now if the probing is able to recognize a 
protocol.
The logic for the format_name in the QEMU existent code (I has kept it here in 
the patch) is a slippery way for an imprudent user. That's why I staked on the 
backing_file_str deprication in the previous version.


2. we can do probing now
3. we can at least check, if backing_file_str == 


Not bad for the sanity check but we will search a node by the file name again - 
not good ((


Not search, but only check one very likely option.

Additionally to 1. or 3. (or combined), or even keeping things as is (i.e. 
wrong, but it is preexisting), we can:

 - add backing-format argument to qapi as pair for backing-file
 - deprecate using backing-file without backing-format.

Then, after deprecation period we'll have correct code. This may be done in 
separate.




base_unfiltered->filename, in this case we can use 
base_unfiltered->drv->format_name



+    }
+    } else {
+    base_unfiltered = bdrv_skip_filters(base);
+    if (base_unfiltered) {
+    base_id = base_unfiltered->filename;
+    if (base_unfiltered->drv) {
+    base_fmt = base_unfiltered->drv->format_name;
+    }
+    }
  }
  }
  bdrv_set_backing_hd(unfiltered_bs, base, _err);
diff --git a/blockdev.c b/blockdev.c
index c917625..0e9c783 100644
--- a/blockdev.c
+++ b/blockdev.c


[...]


-    stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
+    stream_start(has_job_id ? job_id : NULL, bs, base_bs,
+ has_backing_file ? backing_file : NULL,


backing_file should be NULL if has_backing_file is false, so you can use just 
backing_file instead of ternary operator.



Yes, if reliable. I has kept the conformation with the ternary operator at the 
first parameter above.

Andrey


   job_flags, has_speed ? speed : 0, on_error,
   filter_node_name, _err);
  if (local_err) {







--
Best regards,
Vladimir



Re: [PATCH v12 13/14] stream: skip filters when writing backing file name to QCOW2 header

2020-10-27 Thread Andrey Shinkevich

On 27.10.2020 18:09, Vladimir Sementsov-Ogievskiy wrote:

22.10.2020 21:13, Andrey Shinkevich wrote:

Avoid writing a filter JSON file name and a filter format name to QCOW2
image when the backing file is changed after the block stream job.
A user is still able to assign the 'backing-file' parameter for a
block-stream job keeping in mind the possible issue mentioned above.
If the user does not specify the 'backing-file' parameter, QEMU will
assign it automatically.

Signed-off-by: Andrey Shinkevich 
---
  block/stream.c | 15 +--
  blockdev.c |  9 ++---
  2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index e0540ee..1ba74ab 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -65,6 +65,7 @@ static int stream_prepare(Job *job)
  BlockDriverState *bs = blk_bs(bjob->blk);
  BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
  BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
+    BlockDriverState *base_unfiltered = NULL;
  Error *local_err = NULL;
  int ret = 0;
@@ -75,8 +76,18 @@ static int stream_prepare(Job *job)
  const char *base_id = NULL, *base_fmt = NULL;
  if (base) {
  base_id = s->backing_file_str;
-    if (base->drv) {
-    base_fmt = base->drv->format_name;
+    if (base_id) {
+    if (base->drv) {
+    base_fmt = base->drv->format_name;


hmm. this doesn't make real sense: so, we assume that user specified 
backing_file_str, which may not relate to base, but we use 
base->drv->format_name? But it may be name of the filter driver, which 
would be wrong..


Any ideas?

1. we can use base_fmt=NULL, to provoke probing on next open of the 
qcow2 file..


I would choose this item #1 but have to check the probing code logic... 
Particularly, I do not remember now if the probing is able to recognize 
a protocol.
The logic for the format_name in the QEMU existent code (I has kept it 
here in the patch) is a slippery way for an imprudent user. That's why I 
staked on the backing_file_str deprication in the previous version.



2. we can do probing now
3. we can at least check, if backing_file_str == 


Not bad for the sanity check but we will search a node by the file name 
again - not good ((


base_unfiltered->filename, in this case we can use 
base_unfiltered->drv->format_name




+    }
+    } else {
+    base_unfiltered = bdrv_skip_filters(base);
+    if (base_unfiltered) {
+    base_id = base_unfiltered->filename;
+    if (base_unfiltered->drv) {
+    base_fmt = base_unfiltered->drv->format_name;
+    }
+    }
  }
  }
  bdrv_set_backing_hd(unfiltered_bs, base, _err);
diff --git a/blockdev.c b/blockdev.c
index c917625..0e9c783 100644
--- a/blockdev.c
+++ b/blockdev.c


[...]


-    stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
+    stream_start(has_job_id ? job_id : NULL, bs, base_bs,
+ has_backing_file ? backing_file : NULL,


backing_file should be NULL if has_backing_file is false, so you can use 
just backing_file instead of ternary operator.




Yes, if reliable. I has kept the conformation with the ternary operator 
at the first parameter above.


Andrey


   job_flags, has_speed ? speed : 0, on_error,
   filter_node_name, _err);
  if (local_err) {








Re: [PATCH v12 13/14] stream: skip filters when writing backing file name to QCOW2 header

2020-10-27 Thread Vladimir Sementsov-Ogievskiy

22.10.2020 21:13, Andrey Shinkevich wrote:

Avoid writing a filter JSON file name and a filter format name to QCOW2
image when the backing file is changed after the block stream job.
A user is still able to assign the 'backing-file' parameter for a
block-stream job keeping in mind the possible issue mentioned above.
If the user does not specify the 'backing-file' parameter, QEMU will
assign it automatically.

Signed-off-by: Andrey Shinkevich 
---
  block/stream.c | 15 +--
  blockdev.c |  9 ++---
  2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index e0540ee..1ba74ab 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -65,6 +65,7 @@ static int stream_prepare(Job *job)
  BlockDriverState *bs = blk_bs(bjob->blk);
  BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
  BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
+BlockDriverState *base_unfiltered = NULL;
  Error *local_err = NULL;
  int ret = 0;
  
@@ -75,8 +76,18 @@ static int stream_prepare(Job *job)

  const char *base_id = NULL, *base_fmt = NULL;
  if (base) {
  base_id = s->backing_file_str;
-if (base->drv) {
-base_fmt = base->drv->format_name;
+if (base_id) {
+if (base->drv) {
+base_fmt = base->drv->format_name;


hmm. this doesn't make real sense: so, we assume that user specified 
backing_file_str, which may not relate to base, but we use 
base->drv->format_name? But it may be name of the filter driver, which would be 
wrong..

Any ideas?

1. we can use base_fmt=NULL, to provoke probing on next open of the qcow2 file..
2. we can do probing now
3. we can at least check, if backing_file_str == base_unfiltered->filename, in this 
case we can use base_unfiltered->drv->format_name



+}
+} else {
+base_unfiltered = bdrv_skip_filters(base);
+if (base_unfiltered) {
+base_id = base_unfiltered->filename;
+if (base_unfiltered->drv) {
+base_fmt = base_unfiltered->drv->format_name;
+}
+}
  }
  }
  bdrv_set_backing_hd(unfiltered_bs, base, _err);
diff --git a/blockdev.c b/blockdev.c
index c917625..0e9c783 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2508,7 +2508,6 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
  BlockDriverState *base_bs = NULL;
  AioContext *aio_context;
  Error *local_err = NULL;
-const char *base_name = NULL;
  int job_flags = JOB_DEFAULT;
  
  if (!has_on_error) {

@@ -2536,7 +2535,6 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
  goto out;
  }
  assert(bdrv_get_aio_context(base_bs) == aio_context);
-base_name = base;
  }
  
  if (has_base_node) {

@@ -2551,7 +2549,6 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
  }
  assert(bdrv_get_aio_context(base_bs) == aio_context);
  bdrv_refresh_filename(base_bs);
-base_name = base_bs->filename;
  }
  
  /* Check for op blockers in the whole chain between bs and base */

@@ -2571,9 +2568,6 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
  goto out;
  }
  
-/* backing_file string overrides base bs filename */

-base_name = has_backing_file ? backing_file : base_name;
-
  if (has_auto_finalize && !auto_finalize) {
  job_flags |= JOB_MANUAL_FINALIZE;
  }
@@ -2581,7 +2575,8 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
  job_flags |= JOB_MANUAL_DISMISS;
  }
  
-stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,

+stream_start(has_job_id ? job_id : NULL, bs, base_bs,
+ has_backing_file ? backing_file : NULL,


backing_file should be NULL if has_backing_file is false, so you can use just 
backing_file instead of ternary operator.


   job_flags, has_speed ? speed : 0, on_error,
   filter_node_name, _err);
  if (local_err) {




--
Best regards,
Vladimir



[PATCH v12 13/14] stream: skip filters when writing backing file name to QCOW2 header

2020-10-22 Thread Andrey Shinkevich via
Avoid writing a filter JSON file name and a filter format name to QCOW2
image when the backing file is changed after the block stream job.
A user is still able to assign the 'backing-file' parameter for a
block-stream job keeping in mind the possible issue mentioned above.
If the user does not specify the 'backing-file' parameter, QEMU will
assign it automatically.

Signed-off-by: Andrey Shinkevich 
---
 block/stream.c | 15 +--
 blockdev.c |  9 ++---
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index e0540ee..1ba74ab 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -65,6 +65,7 @@ static int stream_prepare(Job *job)
 BlockDriverState *bs = blk_bs(bjob->blk);
 BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
 BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
+BlockDriverState *base_unfiltered = NULL;
 Error *local_err = NULL;
 int ret = 0;
 
@@ -75,8 +76,18 @@ static int stream_prepare(Job *job)
 const char *base_id = NULL, *base_fmt = NULL;
 if (base) {
 base_id = s->backing_file_str;
-if (base->drv) {
-base_fmt = base->drv->format_name;
+if (base_id) {
+if (base->drv) {
+base_fmt = base->drv->format_name;
+}
+} else {
+base_unfiltered = bdrv_skip_filters(base);
+if (base_unfiltered) {
+base_id = base_unfiltered->filename;
+if (base_unfiltered->drv) {
+base_fmt = base_unfiltered->drv->format_name;
+}
+}
 }
 }
 bdrv_set_backing_hd(unfiltered_bs, base, _err);
diff --git a/blockdev.c b/blockdev.c
index c917625..0e9c783 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2508,7 +2508,6 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
 BlockDriverState *base_bs = NULL;
 AioContext *aio_context;
 Error *local_err = NULL;
-const char *base_name = NULL;
 int job_flags = JOB_DEFAULT;
 
 if (!has_on_error) {
@@ -2536,7 +2535,6 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
 goto out;
 }
 assert(bdrv_get_aio_context(base_bs) == aio_context);
-base_name = base;
 }
 
 if (has_base_node) {
@@ -2551,7 +2549,6 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
 }
 assert(bdrv_get_aio_context(base_bs) == aio_context);
 bdrv_refresh_filename(base_bs);
-base_name = base_bs->filename;
 }
 
 /* Check for op blockers in the whole chain between bs and base */
@@ -2571,9 +2568,6 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
 goto out;
 }
 
-/* backing_file string overrides base bs filename */
-base_name = has_backing_file ? backing_file : base_name;
-
 if (has_auto_finalize && !auto_finalize) {
 job_flags |= JOB_MANUAL_FINALIZE;
 }
@@ -2581,7 +2575,8 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
 job_flags |= JOB_MANUAL_DISMISS;
 }
 
-stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
+stream_start(has_job_id ? job_id : NULL, bs, base_bs,
+ has_backing_file ? backing_file : NULL,
  job_flags, has_speed ? speed : 0, on_error,
  filter_node_name, _err);
 if (local_err) {
-- 
1.8.3.1