From: Nikita Lapshin <nikita.lapshin@openvz.org>

qemu_ftell() will return wrong value for non-writable QEMUFile.
This happens due to call qemu_fflush() inside qemu_ftell(), this
function won't flush if file is readable.

Signed-off-by: Nikita Lapshin <nikita.lapshin@openvz.org>
---
 migration/qemu-file.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 1479cddad9..53ccef80ac 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -663,7 +663,8 @@ int64_t qemu_ftell_fast(QEMUFile *f)
 int64_t qemu_ftell(QEMUFile *f)
 {
     qemu_fflush(f);
-    return f->pos;
+    /* Consider that qemu_fflush() won't work if file is non-writable */
+    return f->pos + f->buf_index;
 }
 
 int qemu_file_rate_limit(QEMUFile *f)
-- 
2.31.1

On Thu, Jun 16, 2022 at 01:28:05PM +0300, nikita.lapshin@openvz.org wrote:
> From: Nikita Lapshin <nikita.lapshin@openvz.org>
> 
> qemu_ftell() will return wrong value for non-writable QEMUFile.
> This happens due to call qemu_fflush() inside qemu_ftell(), this
> function won't flush if file is readable.

Well the return value isn't necessarily wrong today - it really
depends what semantics each callers desires.

Can you say what particular caller needs these semantics changed
and the impact on them from current behaviour ?

> Signed-off-by: Nikita Lapshin <nikita.lapshin@openvz.org>
> ---
>  migration/qemu-file.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 1479cddad9..53ccef80ac 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -663,7 +663,8 @@ int64_t qemu_ftell_fast(QEMUFile *f)
>  int64_t qemu_ftell(QEMUFile *f)
>  {
>      qemu_fflush(f);
> -    return f->pos;
> +    /* Consider that qemu_fflush() won't work if file is non-writable */
> +    return f->pos + f->buf_index;
>  }

IIUC, this is more or less trying to make 'qemu_ftell' be
equivalent to 'qemu_ftell_fast' semantics in the non-writable
case. But that makes me wonder if whichever calls has problems,
shouldn't be just changed to use  qemu_ftell_fast instead ?


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 6/16/22 2:20 PM, Daniel P. Berrangé wrote:
> On Thu, Jun 16, 2022 at 01:28:05PM +0300, nikita.lapshin@openvz.org wrote:
>> From: Nikita Lapshin <nikita.lapshin@openvz.org>
>>
>> qemu_ftell() will return wrong value for non-writable QEMUFile.
>> This happens due to call qemu_fflush() inside qemu_ftell(), this
>> function won't flush if file is readable.
> Well the return value isn't necessarily wrong today - it really
> depends what semantics each callers desires.
>
> Can you say what particular caller needs these semantics changed
> and the impact on them from current behaviour ?

Sorry, it's my bad. It was used in previous version. But after 
refactoring I threw this function out of implementation.

So it is in fact not used now.

>
>> Signed-off-by: Nikita Lapshin <nikita.lapshin@openvz.org>
>> ---
>>   migration/qemu-file.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
>> index 1479cddad9..53ccef80ac 100644
>> --- a/migration/qemu-file.c
>> +++ b/migration/qemu-file.c
>> @@ -663,7 +663,8 @@ int64_t qemu_ftell_fast(QEMUFile *f)
>>   int64_t qemu_ftell(QEMUFile *f)
>>   {
>>       qemu_fflush(f);
>> -    return f->pos;
>> +    /* Consider that qemu_fflush() won't work if file is non-writable */
>> +    return f->pos + f->buf_index;
>>   }
> IIUC, this is more or less trying to make 'qemu_ftell' be
> equivalent to 'qemu_ftell_fast' semantics in the non-writable
> case. But that makes me wonder if whichever calls has problems,
> shouldn't be just changed to use  qemu_ftell_fast instead ?

qemu_ftell_fast() counts iovec length. When we try to read from QEMUFile 
using for exmaple qemu_peek_buffer f->buf will be filled with data so we 
need to consider buf_index.

>
> With regards,
> Daniel
Thank you for your review!