When the "file:" migration support was added we missed the special
case in the qemu_open_old implementation that allows for a particular
file name format to be used to refer to a set of file descriptors that
have been previously provided to QEMU via the add-fd QMP command.

When using this fdset feature, we should not truncate the migration
file because being given an fd means that the management layer is in
control of the file and will likely already have some data written to
it. This is further indicated by the presence of the 'offset'
argument, which indicates the start of the region where QEMU is
allowed to write.

Fix the issue by replacing the O_TRUNC flag on open by an ftruncate
call, which will take the offset into consideration.

Fixes: 385f510df5 ("migration: file URI offset")
Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/file.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/migration/file.c b/migration/file.c
index ab18ba505a..ba5b5c44ff 100644
--- a/migration/file.c
+++ b/migration/file.c
@@ -84,12 +84,19 @@ void file_start_outgoing_migration(MigrationState *s,
 
     trace_migration_file_outgoing(filename);
 
-    fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC,
-                                     0600, errp);
+    fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY, 0600, errp);
     if (!fioc) {
         return;
     }
 
+    if (ftruncate(fioc->fd, offset)) {
+        error_setg_errno(errp, errno,
+                         "failed to truncate migration file to offset %" PRIx64,
+                         offset);
+        object_unref(OBJECT(fioc));
+        return;
+    }
+
     outgoing_args.fname = g_strdup(filename);
 
     ioc = QIO_CHANNEL(fioc);
-- 
2.35.3

On Thu, May 23, 2024 at 04:05:31PM -0300, Fabiano Rosas wrote:
> When the "file:" migration support was added we missed the special
> case in the qemu_open_old implementation that allows for a particular
> file name format to be used to refer to a set of file descriptors that
> have been previously provided to QEMU via the add-fd QMP command.
> 
> When using this fdset feature, we should not truncate the migration
> file because being given an fd means that the management layer is in
> control of the file and will likely already have some data written to
> it. This is further indicated by the presence of the 'offset'
> argument, which indicates the start of the region where QEMU is
> allowed to write.
> 
> Fix the issue by replacing the O_TRUNC flag on open by an ftruncate
> call, which will take the offset into consideration.
> 
> Fixes: 385f510df5 ("migration: file URI offset")
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/file.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 Thu, May 23, 2024 at 04:05:31PM -0300, Fabiano Rosas wrote:
> When the "file:" migration support was added we missed the special
> case in the qemu_open_old implementation that allows for a particular
> file name format to be used to refer to a set of file descriptors that
> have been previously provided to QEMU via the add-fd QMP command.
> 
> When using this fdset feature, we should not truncate the migration
> file because being given an fd means that the management layer is in
> control of the file and will likely already have some data written to
> it. This is further indicated by the presence of the 'offset'
> argument, which indicates the start of the region where QEMU is
> allowed to write.
> 
> Fix the issue by replacing the O_TRUNC flag on open by an ftruncate
> call, which will take the offset into consideration.
> 
> Fixes: 385f510df5 ("migration: file URI offset")
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

Right below the change, did we forget to free the ioc if
qio_channel_io_seek() failed?

> ---
>  migration/file.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/file.c b/migration/file.c
> index ab18ba505a..ba5b5c44ff 100644
> --- a/migration/file.c
> +++ b/migration/file.c
> @@ -84,12 +84,19 @@ void file_start_outgoing_migration(MigrationState *s,
>  
>      trace_migration_file_outgoing(filename);
>  
> -    fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC,
> -                                     0600, errp);
> +    fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY, 0600, errp);
>      if (!fioc) {
>          return;
>      }
>  
> +    if (ftruncate(fioc->fd, offset)) {
> +        error_setg_errno(errp, errno,
> +                         "failed to truncate migration file to offset %" PRIx64,
> +                         offset);
> +        object_unref(OBJECT(fioc));
> +        return;
> +    }
> +
>      outgoing_args.fname = g_strdup(filename);
>  
>      ioc = QIO_CHANNEL(fioc);
> -- 
> 2.35.3
> 

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Thu, May 23, 2024 at 04:05:31PM -0300, Fabiano Rosas wrote:
>> When the "file:" migration support was added we missed the special
>> case in the qemu_open_old implementation that allows for a particular
>> file name format to be used to refer to a set of file descriptors that
>> have been previously provided to QEMU via the add-fd QMP command.
>> 
>> When using this fdset feature, we should not truncate the migration
>> file because being given an fd means that the management layer is in
>> control of the file and will likely already have some data written to
>> it. This is further indicated by the presence of the 'offset'
>> argument, which indicates the start of the region where QEMU is
>> allowed to write.
>> 
>> Fix the issue by replacing the O_TRUNC flag on open by an ftruncate
>> call, which will take the offset into consideration.
>> 
>> Fixes: 385f510df5 ("migration: file URI offset")
>> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>
> Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks

>
> Right below the change, did we forget to free the ioc if
> qio_channel_io_seek() failed?
>

Yep, looks like it. I'll fix it.

>> ---
>>  migration/file.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>> 
>> diff --git a/migration/file.c b/migration/file.c
>> index ab18ba505a..ba5b5c44ff 100644
>> --- a/migration/file.c
>> +++ b/migration/file.c
>> @@ -84,12 +84,19 @@ void file_start_outgoing_migration(MigrationState *s,
>>  
>>      trace_migration_file_outgoing(filename);
>>  
>> -    fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC,
>> -                                     0600, errp);
>> +    fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY, 0600, errp);
>>      if (!fioc) {
>>          return;
>>      }
>>  
>> +    if (ftruncate(fioc->fd, offset)) {
>> +        error_setg_errno(errp, errno,
>> +                         "failed to truncate migration file to offset %" PRIx64,
>> +                         offset);
>> +        object_unref(OBJECT(fioc));
>> +        return;
>> +    }
>> +
>>      outgoing_args.fname = g_strdup(filename);
>>  
>>      ioc = QIO_CHANNEL(fioc);
>> -- 
>> 2.35.3
>> 

On Fri, 24 May 2024 at 00:38, Fabiano Rosas <farosas@suse.de> wrote:
> This is further indicated by the presence of the 'offset'
> argument, which indicates the start of the region where QEMU is
> allowed to write.
>
> Fix the issue by replacing the O_TRUNC flag on open by an ftruncate
> call, which will take the offset into consideration.
>
> +    if (ftruncate(fioc->fd, offset)) {
> +        error_setg_errno(errp, errno,
> +                         "failed to truncate migration file to offset %" PRIx64,
> +                         offset);
> +        object_unref(OBJECT(fioc));
> +        return;
> +    }
> +

* Should 'offset' be checked for > zero while ftruncating? Else it'll
be same as O_TRUNC. Otherwise it looks fine.

Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>

Thank you.
---
  - Prasad

Prasad Pandit <ppandit@redhat.com> writes:

> On Fri, 24 May 2024 at 00:38, Fabiano Rosas <farosas@suse.de> wrote:
>> This is further indicated by the presence of the 'offset'
>> argument, which indicates the start of the region where QEMU is
>> allowed to write.
>>
>> Fix the issue by replacing the O_TRUNC flag on open by an ftruncate
>> call, which will take the offset into consideration.
>>
>> +    if (ftruncate(fioc->fd, offset)) {
>> +        error_setg_errno(errp, errno,
>> +                         "failed to truncate migration file to offset %" PRIx64,
>> +                         offset);
>> +        object_unref(OBJECT(fioc));
>> +        return;
>> +    }
>> +
>
> * Should 'offset' be checked for > zero while ftruncating? Else it'll
> be same as O_TRUNC. Otherwise it looks fine.

That's the point. If offset==0 we truncate all the way, if not, we
truncate to the offset.

>
> Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>

Thanks!

On Fri, 24 May 2024 at 18:00, Fabiano Rosas <farosas@suse.de> wrote:
> That's the point. If offset==0 we truncate all the way, if not, we truncate to the offset.

* Yes, I was wondering if the migration file has some data, but still
'offset' ends up being zero(0). If that's unlikely to happen, then we
are good.

Thank you.
---
  - Prasad