src/qemu/qemu_hotplug.c | 3 +++ src/qemu/qemu_process.c | 26 ++++++++++++++++++++++++++ src/qemu/qemu_process.h | 4 ++++ 3 files changed, 33 insertions(+)
https://bugzilla.redhat.com/show_bug.cgi?id=1461214
Since fec8f9c49af we try to use predictable file names for
'memory-backend-file' objects. But that made us provide full path
to qemu when hot plugging the object while previously we provided
merely a directory. But this makes qemu behave differently. If
qemu sees a path terminated with a directory it calls mkstemp()
and unlinks the file immediately. But if it sees full path it
just calls open(path, O_CREAT ..); and never unlinks the file.
Therefore it's up to libvirt to unlink the file and not leave it
behind.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
Zack, can you please check if this patch is suitable for your use cases?
src/qemu/qemu_hotplug.c | 3 +++
src/qemu/qemu_process.c | 26 ++++++++++++++++++++++++++
src/qemu/qemu_process.h | 4 ++++
3 files changed, 33 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 6dc16a105..f26e2ca60 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3894,6 +3894,9 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver,
if (qemuDomainNamespaceTeardownMemory(vm, mem) < 0)
VIR_WARN("Unable to remove memory device from /dev");
+ if (qemuProcessDestroyMemoryBackingPath(driver, vm, mem) < 0)
+ VIR_WARN("Unable to destroy memory backing path");
+
virDomainMemoryDefFree(mem);
/* fix the balloon size */
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1a0923af3..73624eefe 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3467,6 +3467,32 @@ qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver,
}
+int
+qemuProcessDestroyMemoryBackingPath(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainMemoryDefPtr mem)
+{
+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ char *path = NULL;
+ int ret = -1;
+
+ if (qemuGetMemoryBackingPath(vm->def, cfg, mem->info.alias, &path) < 0)
+ goto cleanup;
+
+ if (unlink(path) < 0 &&
+ errno != ENOENT) {
+ virReportSystemError(errno, _("Unable to remove %s"), path);
+ goto cleanup;
+ }
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(path);
+ virObjectUnref(cfg);
+ return ret;
+}
+
+
static int
qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver,
virDomainGraphicsDefPtr graphics,
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index cd9a72031..3fc7d6c85 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -43,6 +43,10 @@ int qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver,
virDomainMemoryDefPtr mem,
bool build);
+int qemuProcessDestroyMemoryBackingPath(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainMemoryDefPtr mem);
+
void qemuProcessAutostartAll(virQEMUDriverPtr driver);
void qemuProcessReconnectAll(virConnectPtr conn, virQEMUDriverPtr driver);
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jan 11, 2018 at 01:24:57PM +0100, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1461214
>
> Since fec8f9c49af we try to use predictable file names for
> 'memory-backend-file' objects. But that made us provide full path
> to qemu when hot plugging the object while previously we provided
> merely a directory. But this makes qemu behave differently. If
> qemu sees a path terminated with a directory it calls mkstemp()
> and unlinks the file immediately. But if it sees full path it
> just calls open(path, O_CREAT ..); and never unlinks the file.
> Therefore it's up to libvirt to unlink the file and not leave it
> behind.
IIUC, this unlinks the file when QEMU shuts down (or the mem device is
unplugged).
Is there any benefit and/or downside to doing the same as QEMU and
unlinking it immediately after QEMU has opened it ? I guess figuring
out the right time might be hard todo race-free.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>
> Zack, can you please check if this patch is suitable for your use cases?
>
> src/qemu/qemu_hotplug.c | 3 +++
> src/qemu/qemu_process.c | 26 ++++++++++++++++++++++++++
> src/qemu/qemu_process.h | 4 ++++
> 3 files changed, 33 insertions(+)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 6dc16a105..f26e2ca60 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3894,6 +3894,9 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver,
> if (qemuDomainNamespaceTeardownMemory(vm, mem) < 0)
> VIR_WARN("Unable to remove memory device from /dev");
>
> + if (qemuProcessDestroyMemoryBackingPath(driver, vm, mem) < 0)
> + VIR_WARN("Unable to destroy memory backing path");
> +
> virDomainMemoryDefFree(mem);
>
> /* fix the balloon size */
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 1a0923af3..73624eefe 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3467,6 +3467,32 @@ qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver,
> }
>
>
> +int
> +qemuProcessDestroyMemoryBackingPath(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + virDomainMemoryDefPtr mem)
> +{
> + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> + char *path = NULL;
> + int ret = -1;
> +
> + if (qemuGetMemoryBackingPath(vm->def, cfg, mem->info.alias, &path) < 0)
> + goto cleanup;
> +
> + if (unlink(path) < 0 &&
> + errno != ENOENT) {
> + virReportSystemError(errno, _("Unable to remove %s"), path);
> + goto cleanup;
> + }
> +
> + ret = 0;
> + cleanup:
> + VIR_FREE(path);
> + virObjectUnref(cfg);
> + return ret;
> +}
> +
> +
> static int
> qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver,
> virDomainGraphicsDefPtr graphics,
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index cd9a72031..3fc7d6c85 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -43,6 +43,10 @@ int qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver,
> virDomainMemoryDefPtr mem,
> bool build);
>
> +int qemuProcessDestroyMemoryBackingPath(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + virDomainMemoryDefPtr mem);
> +
> void qemuProcessAutostartAll(virQEMUDriverPtr driver);
> void qemuProcessReconnectAll(virConnectPtr conn, virQEMUDriverPtr driver);
>
> --
> 2.13.6
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 01/11/2018 01:32 PM, Daniel P. Berrange wrote: > On Thu, Jan 11, 2018 at 01:24:57PM +0100, Michal Privoznik wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1461214 >> >> Since fec8f9c49af we try to use predictable file names for >> 'memory-backend-file' objects. But that made us provide full path >> to qemu when hot plugging the object while previously we provided >> merely a directory. But this makes qemu behave differently. If >> qemu sees a path terminated with a directory it calls mkstemp() >> and unlinks the file immediately. But if it sees full path it >> just calls open(path, O_CREAT ..); and never unlinks the file. >> Therefore it's up to libvirt to unlink the file and not leave it >> behind. > > IIUC, this unlinks the file when QEMU shuts down (or the mem device is > unplugged). > > Is there any benefit and/or downside to doing the same as QEMU and > unlinking it immediately after QEMU has opened it ? I guess figuring > out the right time might be hard todo race-free. Yeah, I thought about that race too. Also, since we use memory-backing-file whenever memoryBacking/access/@mode='shared' or memoryBacking/source/@type='file' is used I figured that whoever set those might want to do something with the files (e.g. read them?). But if we unlink them right away we would be only making it harder for users to get to the files. But I don't have strong preference either way. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jan 11, 2018 at 13:24:57 +0100, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1461214
>
> Since fec8f9c49af we try to use predictable file names for
> 'memory-backend-file' objects. But that made us provide full path
> to qemu when hot plugging the object while previously we provided
> merely a directory. But this makes qemu behave differently. If
> qemu sees a path terminated with a directory it calls mkstemp()
> and unlinks the file immediately. But if it sees full path it
> just calls open(path, O_CREAT ..); and never unlinks the file.
> Therefore it's up to libvirt to unlink the file and not leave it
> behind.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>
> Zack, can you please check if this patch is suitable for your use cases?
>
> src/qemu/qemu_hotplug.c | 3 +++
> src/qemu/qemu_process.c | 26 ++++++++++++++++++++++++++
> src/qemu/qemu_process.h | 4 ++++
> 3 files changed, 33 insertions(+)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 6dc16a105..f26e2ca60 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3894,6 +3894,9 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver,
> if (qemuDomainNamespaceTeardownMemory(vm, mem) < 0)
> VIR_WARN("Unable to remove memory device from /dev");
>
> + if (qemuProcessDestroyMemoryBackingPath(driver, vm, mem) < 0)
> + VIR_WARN("Unable to destroy memory backing path");
> +
> virDomainMemoryDefFree(mem);
This will not call the function when we shut down qemu. Is the full
directory removed in that case?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 01/11/2018 01:36 PM, Peter Krempa wrote:
> On Thu, Jan 11, 2018 at 13:24:57 +0100, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1461214
>>
>> Since fec8f9c49af we try to use predictable file names for
>> 'memory-backend-file' objects. But that made us provide full path
>> to qemu when hot plugging the object while previously we provided
>> merely a directory. But this makes qemu behave differently. If
>> qemu sees a path terminated with a directory it calls mkstemp()
>> and unlinks the file immediately. But if it sees full path it
>> just calls open(path, O_CREAT ..); and never unlinks the file.
>> Therefore it's up to libvirt to unlink the file and not leave it
>> behind.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>
>> Zack, can you please check if this patch is suitable for your use cases?
>>
>> src/qemu/qemu_hotplug.c | 3 +++
>> src/qemu/qemu_process.c | 26 ++++++++++++++++++++++++++
>> src/qemu/qemu_process.h | 4 ++++
>> 3 files changed, 33 insertions(+)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 6dc16a105..f26e2ca60 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -3894,6 +3894,9 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver,
>> if (qemuDomainNamespaceTeardownMemory(vm, mem) < 0)
>> VIR_WARN("Unable to remove memory device from /dev");
>>
>> + if (qemuProcessDestroyMemoryBackingPath(driver, vm, mem) < 0)
>> + VIR_WARN("Unable to destroy memory backing path");
>> +
>> virDomainMemoryDefFree(mem);
>
> This will not call the function when we shut down qemu. Is the full
> directory removed in that case?
>
Yes, from qemuProcessStop() we call qemuProcessBuildDestroyMemoryPaths()
which in turn calls virFileDeleteTree() over all domain specific
hugepages paths and memoryBacking ones.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
----- Original Message ----- > From: "Michal Privoznik" <mprivozn@redhat.com> > To: libvir-list@redhat.com > Cc: "Zack Cornelius" <zack.cornelius@kove.net> > Sent: Thursday, January 11, 2018 6:24:57 AM > Subject: [PATCH] qemuDomainRemoveMemoryDevice: unlink() memory backing file > > Zack, can you please check if this patch is suitable for your use cases? > This patch will work for Kove's use cases. --Zack -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/11/2018 01:24 PM, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1461214 > > Since fec8f9c49af we try to use predictable file names for > 'memory-backend-file' objects. But that made us provide full path > to qemu when hot plugging the object while previously we provided > merely a directory. But this makes qemu behave differently. If > qemu sees a path terminated with a directory it calls mkstemp() > and unlinks the file immediately. But if it sees full path it > just calls open(path, O_CREAT ..); and never unlinks the file. > Therefore it's up to libvirt to unlink the file and not leave it > behind. > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > --- > > Zack, can you please check if this patch is suitable for your use cases? > > src/qemu/qemu_hotplug.c | 3 +++ > src/qemu/qemu_process.c | 26 ++++++++++++++++++++++++++ > src/qemu/qemu_process.h | 4 ++++ > 3 files changed, 33 insertions(+) > Ping. Are there any other questions I can answer? Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jan 11, 2018 at 13:24:57 +0100, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1461214 > > Since fec8f9c49af we try to use predictable file names for > 'memory-backend-file' objects. But that made us provide full path > to qemu when hot plugging the object while previously we provided > merely a directory. But this makes qemu behave differently. If > qemu sees a path terminated with a directory it calls mkstemp() > and unlinks the file immediately. But if it sees full path it > just calls open(path, O_CREAT ..); and never unlinks the file. > Therefore it's up to libvirt to unlink the file and not leave it > behind. > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > --- > > Zack, can you please check if this patch is suitable for your use cases? > > src/qemu/qemu_hotplug.c | 3 +++ > src/qemu/qemu_process.c | 26 ++++++++++++++++++++++++++ > src/qemu/qemu_process.h | 4 ++++ > 3 files changed, 33 insertions(+) ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/02/2018 11:01 AM, Jiri Denemark wrote: > On Thu, Jan 11, 2018 at 13:24:57 +0100, Michal Privoznik wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1461214 >> >> Since fec8f9c49af we try to use predictable file names for >> 'memory-backend-file' objects. But that made us provide full path >> to qemu when hot plugging the object while previously we provided >> merely a directory. But this makes qemu behave differently. If >> qemu sees a path terminated with a directory it calls mkstemp() >> and unlinks the file immediately. But if it sees full path it >> just calls open(path, O_CREAT ..); and never unlinks the file. >> Therefore it's up to libvirt to unlink the file and not leave it >> behind. >> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> >> --- >> >> Zack, can you please check if this patch is suitable for your use cases? >> >> src/qemu/qemu_hotplug.c | 3 +++ >> src/qemu/qemu_process.c | 26 ++++++++++++++++++++++++++ >> src/qemu/qemu_process.h | 4 ++++ >> 3 files changed, 33 insertions(+) > > ACK > > Jirka > Thanks, pushed. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.