Re: [libvirt] [PATCH v2 2/4] qemu: block: use the delete flag to delete snapshot images if requested
On Thu, Nov 21, 2019 at 09:31:41 +0100, Pavel Mores wrote:
> On Wed, Nov 20, 2019 at 12:11:32PM +0100, Peter Krempa wrote:
> > On Wed, Nov 20, 2019 at 11:44:53 +0100, Pavel Mores wrote:
> > > When blockcommit finishes successfully, one of the
> > > qemuBlockJobProcessEventCompletedCommit() and
> > > qemuBlockJobProcessEventCompletedActiveCommit() event handlers is called.
> > > This is where the delete flag (stored in qemuBlockJobCommitData since the
> > > previous commit) can actually be used to delete the committed snapshot
> > > images if requested.
> > >
> > > Signed-off-by: Pavel Mores
> > > ---
> > > src/qemu/qemu_blockjob.c | 31 +++
> > > 1 file changed, 31 insertions(+)
> > >
> > > diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> > > index 7d94a6ce38..cf221a2839 100644
> > > --- a/src/qemu/qemu_blockjob.c
> > > +++ b/src/qemu/qemu_blockjob.c
> > > @@ -965,6 +965,31 @@
> > > qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver,
> > > }
> > >
> > >
> > > +/**
> > > + * Helper for qemuBlockJobProcessEventCompletedCommit() and
> > > + * qemuBlockJobProcessEventCompletedActiveCommit(). Relies on
> > > adjustments
> > > + * these functions perform on the 'backingStore' chain to function
> > > correctly.
> > > + *
> > > + * TODO look into removing backing store for non-local snapshots too
> > > + */
> > > +static void
> > > +qemuBlockJobUnlinkCommittedStorage(virStorageSourcePtr top)
> > > +{
> > > +virStorageSourcePtr p = top;
> > > +const size_t errmsg_len = 128;
> > > +char errmsg_buf[errmsg_len];
> > > +char *errmsg;
> > > +
> > > +for (; p != NULL; p = p->backingStore) {
> > > +if (virStorageSourceIsLocalStorage(p))
> >
> > The above condition has a multiline body, so it must be enclosed in a
> > block.
> >
> > > +if (unlink(p->path) < 0) {
> >
> > I was considering whether we should use virFileRemove which will also
> > work properly on root-squashed NFS. I'm not sure though how easy it will
> > be to figure out the correct uid and gid inside this helper.
> >
> > If you are interrested into investigating if it's possible, there should
> > be some prior art in the qemu driver where we try to get the uid/gid frm
> > virStorageSource if it's configured, then fall back to the domain
> > uid/gid of the process.
> >
> > > +errmsg = strerror_r(errno, errmsg_buf, errmsg_len);
> >
> > Please use g_strerror(); It does not require any of the buffers and
> > stuff.
> >
> > > +VIR_WARN("Unable to remove snapshot image file %s (%s)",
> > > + p->path, errmsg);
> > > +}
> > > +}
> >
> > The rest looks good.
>
> One more thing comes to my mind though - is VIR_WARN() enough as far as
> reporting the error goes? Would it make sense to replace it with e.g.
> virReportError()?
Usually we'd want a virReportError. This case is special. The block job
handlers are executed from the qemu event handler thread which is not
connected to any API which executes. That means there is nobody to
report the error to.
Additionally the error of deleting the file itself is only minor. The
block job still completed successfully. Even if we could propagate the
error to the user it would make the user think the job failed, but in
fact the data was already copied and we can't undo that. You can see
that we also don't report errors from unplugging of the file itself.
There isn't anything we can do about that if it fails.
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/4] qemu: block: use the delete flag to delete snapshot images if requested
On Wed, Nov 20, 2019 at 12:11:32PM +0100, Peter Krempa wrote:
> On Wed, Nov 20, 2019 at 11:44:53 +0100, Pavel Mores wrote:
> > When blockcommit finishes successfully, one of the
> > qemuBlockJobProcessEventCompletedCommit() and
> > qemuBlockJobProcessEventCompletedActiveCommit() event handlers is called.
> > This is where the delete flag (stored in qemuBlockJobCommitData since the
> > previous commit) can actually be used to delete the committed snapshot
> > images if requested.
> >
> > Signed-off-by: Pavel Mores
> > ---
> > src/qemu/qemu_blockjob.c | 31 +++
> > 1 file changed, 31 insertions(+)
> >
> > diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> > index 7d94a6ce38..cf221a2839 100644
> > --- a/src/qemu/qemu_blockjob.c
> > +++ b/src/qemu/qemu_blockjob.c
> > @@ -965,6 +965,31 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr
> > driver,
> > }
> >
> >
> > +/**
> > + * Helper for qemuBlockJobProcessEventCompletedCommit() and
> > + * qemuBlockJobProcessEventCompletedActiveCommit(). Relies on adjustments
> > + * these functions perform on the 'backingStore' chain to function
> > correctly.
> > + *
> > + * TODO look into removing backing store for non-local snapshots too
> > + */
> > +static void
> > +qemuBlockJobUnlinkCommittedStorage(virStorageSourcePtr top)
> > +{
> > +virStorageSourcePtr p = top;
> > +const size_t errmsg_len = 128;
> > +char errmsg_buf[errmsg_len];
> > +char *errmsg;
> > +
> > +for (; p != NULL; p = p->backingStore) {
> > +if (virStorageSourceIsLocalStorage(p))
>
> The above condition has a multiline body, so it must be enclosed in a
> block.
>
> > +if (unlink(p->path) < 0) {
>
> I was considering whether we should use virFileRemove which will also
> work properly on root-squashed NFS. I'm not sure though how easy it will
> be to figure out the correct uid and gid inside this helper.
>
> If you are interrested into investigating if it's possible, there should
> be some prior art in the qemu driver where we try to get the uid/gid frm
> virStorageSource if it's configured, then fall back to the domain
> uid/gid of the process.
>
> > +errmsg = strerror_r(errno, errmsg_buf, errmsg_len);
>
> Please use g_strerror(); It does not require any of the buffers and
> stuff.
>
> > +VIR_WARN("Unable to remove snapshot image file %s (%s)",
> > + p->path, errmsg);
> > +}
> > +}
>
> The rest looks good.
One more thing comes to my mind though - is VIR_WARN() enough as far as
reporting the error goes? Would it make sense to replace it with e.g.
virReportError()?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/4] qemu: block: use the delete flag to delete snapshot images if requested
On Wed, Nov 20, 2019 at 14:56:18 +0100, Pavel Mores wrote:
> On Wed, Nov 20, 2019 at 12:11:32PM +0100, Peter Krempa wrote:
> > On Wed, Nov 20, 2019 at 11:44:53 +0100, Pavel Mores wrote:
> > > When blockcommit finishes successfully, one of the
> > > qemuBlockJobProcessEventCompletedCommit() and
> > > qemuBlockJobProcessEventCompletedActiveCommit() event handlers is called.
> > > This is where the delete flag (stored in qemuBlockJobCommitData since the
> > > previous commit) can actually be used to delete the committed snapshot
> > > images if requested.
> > >
> > > Signed-off-by: Pavel Mores
> > > ---
> > > src/qemu/qemu_blockjob.c | 31 +++
> > > 1 file changed, 31 insertions(+)
> > >
> > > diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> > > index 7d94a6ce38..cf221a2839 100644
> > > --- a/src/qemu/qemu_blockjob.c
> > > +++ b/src/qemu/qemu_blockjob.c
> > > @@ -965,6 +965,31 @@
> > > qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver,
> > > }
> > >
> > >
> > > +/**
> > > + * Helper for qemuBlockJobProcessEventCompletedCommit() and
> > > + * qemuBlockJobProcessEventCompletedActiveCommit(). Relies on
> > > adjustments
> > > + * these functions perform on the 'backingStore' chain to function
> > > correctly.
> > > + *
> > > + * TODO look into removing backing store for non-local snapshots too
> > > + */
> > > +static void
> > > +qemuBlockJobUnlinkCommittedStorage(virStorageSourcePtr top)
> > > +{
> > > +virStorageSourcePtr p = top;
> > > +const size_t errmsg_len = 128;
> > > +char errmsg_buf[errmsg_len];
> > > +char *errmsg;
> > > +
> > > +for (; p != NULL; p = p->backingStore) {
> > > +if (virStorageSourceIsLocalStorage(p))
> >
> > The above condition has a multiline body, so it must be enclosed in a
> > block.
> >
> > > +if (unlink(p->path) < 0) {
> >
> > I was considering whether we should use virFileRemove which will also
> > work properly on root-squashed NFS. I'm not sure though how easy it will
> > be to figure out the correct uid and gid inside this helper.
> >
> > If you are interrested into investigating if it's possible, there should
> > be some prior art in the qemu driver where we try to get the uid/gid frm
> > virStorageSource if it's configured, then fall back to the domain
> > uid/gid of the process.
>
> I can see qemuDomainGetImageIds() in qemu_domain.c which seems somewhat close
yes, this is the one I meant! I couldn't remember :D
> to what you describe. It tries to get uid/gid from the virStorageSourcePtr
> itself, then its parent virStorageSourcePtr, then virDomainObjPtr and
> finally virQEMUDriverConfigPtr.
That is the correct hierarchy. An image file can either have it's own
security label, or the security label of the disk applies to all of the
chain members, if the disk doesn't have a label there is a default label
for the whole VM and in case that isn't available in case of the DAC
driver we apply the configured uid/gid.
> It's static in its file though, and I don't immediately see an easy way to
> pass
> it the virQEMUDriverConfigPtr. However, if it's OK to make it visible from
I think it's okay to export it.
> blockdev code, I could still call it along the lines of
You can get the config via:
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
and driver via:
virQEMUDriverPtr driver = priv->driver;
and finally:
qemuDomainObjPrivatePtr priv = vm->privateData;
Since qemuDomainGetImageIds already takes vm, I'd suggest you refactor
it or add a wrapper which will not require passing of 'cfg', but will
acquire it via the above mentioned method from vm, so that we don't have
to do it in the blockjob code.
> qemuDomainGetImageIds(NULL, vm, p, NULL, , );
>
> as I can apparently get a virDomainObjPtr into the helper quite easily.
>
> > > +errmsg = strerror_r(errno, errmsg_buf, errmsg_len);
> >
> > Please use g_strerror(); It does not require any of the buffers and
> > stuff.
> >
> > > +VIR_WARN("Unable to remove snapshot image file %s (%s)",
> > > + p->path, errmsg);
> > > +}
> > > +}
> >
> > The rest looks good.
> >
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/4] qemu: block: use the delete flag to delete snapshot images if requested
On Wed, Nov 20, 2019 at 12:11:32PM +0100, Peter Krempa wrote:
> On Wed, Nov 20, 2019 at 11:44:53 +0100, Pavel Mores wrote:
> > When blockcommit finishes successfully, one of the
> > qemuBlockJobProcessEventCompletedCommit() and
> > qemuBlockJobProcessEventCompletedActiveCommit() event handlers is called.
> > This is where the delete flag (stored in qemuBlockJobCommitData since the
> > previous commit) can actually be used to delete the committed snapshot
> > images if requested.
> >
> > Signed-off-by: Pavel Mores
> > ---
> > src/qemu/qemu_blockjob.c | 31 +++
> > 1 file changed, 31 insertions(+)
> >
> > diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> > index 7d94a6ce38..cf221a2839 100644
> > --- a/src/qemu/qemu_blockjob.c
> > +++ b/src/qemu/qemu_blockjob.c
> > @@ -965,6 +965,31 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr
> > driver,
> > }
> >
> >
> > +/**
> > + * Helper for qemuBlockJobProcessEventCompletedCommit() and
> > + * qemuBlockJobProcessEventCompletedActiveCommit(). Relies on adjustments
> > + * these functions perform on the 'backingStore' chain to function
> > correctly.
> > + *
> > + * TODO look into removing backing store for non-local snapshots too
> > + */
> > +static void
> > +qemuBlockJobUnlinkCommittedStorage(virStorageSourcePtr top)
> > +{
> > +virStorageSourcePtr p = top;
> > +const size_t errmsg_len = 128;
> > +char errmsg_buf[errmsg_len];
> > +char *errmsg;
> > +
> > +for (; p != NULL; p = p->backingStore) {
> > +if (virStorageSourceIsLocalStorage(p))
>
> The above condition has a multiline body, so it must be enclosed in a
> block.
>
> > +if (unlink(p->path) < 0) {
>
> I was considering whether we should use virFileRemove which will also
> work properly on root-squashed NFS. I'm not sure though how easy it will
> be to figure out the correct uid and gid inside this helper.
>
> If you are interrested into investigating if it's possible, there should
> be some prior art in the qemu driver where we try to get the uid/gid frm
> virStorageSource if it's configured, then fall back to the domain
> uid/gid of the process.
I can see qemuDomainGetImageIds() in qemu_domain.c which seems somewhat close
to what you describe. It tries to get uid/gid from the virStorageSourcePtr
itself, then its parent virStorageSourcePtr, then virDomainObjPtr and
finally virQEMUDriverConfigPtr.
It's static in its file though, and I don't immediately see an easy way to pass
it the virQEMUDriverConfigPtr. However, if it's OK to make it visible from
blockdev code, I could still call it along the lines of
qemuDomainGetImageIds(NULL, vm, p, NULL, , );
as I can apparently get a virDomainObjPtr into the helper quite easily.
> > +errmsg = strerror_r(errno, errmsg_buf, errmsg_len);
>
> Please use g_strerror(); It does not require any of the buffers and
> stuff.
>
> > +VIR_WARN("Unable to remove snapshot image file %s (%s)",
> > + p->path, errmsg);
> > +}
> > +}
>
> The rest looks good.
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/4] qemu: block: use the delete flag to delete snapshot images if requested
On Wed, Nov 20, 2019 at 11:44:53 +0100, Pavel Mores wrote:
> When blockcommit finishes successfully, one of the
> qemuBlockJobProcessEventCompletedCommit() and
> qemuBlockJobProcessEventCompletedActiveCommit() event handlers is called.
> This is where the delete flag (stored in qemuBlockJobCommitData since the
> previous commit) can actually be used to delete the committed snapshot
> images if requested.
>
> Signed-off-by: Pavel Mores
> ---
> src/qemu/qemu_blockjob.c | 31 +++
> 1 file changed, 31 insertions(+)
>
> diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> index 7d94a6ce38..cf221a2839 100644
> --- a/src/qemu/qemu_blockjob.c
> +++ b/src/qemu/qemu_blockjob.c
> @@ -965,6 +965,31 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr
> driver,
> }
>
>
> +/**
> + * Helper for qemuBlockJobProcessEventCompletedCommit() and
> + * qemuBlockJobProcessEventCompletedActiveCommit(). Relies on adjustments
> + * these functions perform on the 'backingStore' chain to function correctly.
> + *
> + * TODO look into removing backing store for non-local snapshots too
> + */
> +static void
> +qemuBlockJobUnlinkCommittedStorage(virStorageSourcePtr top)
> +{
> +virStorageSourcePtr p = top;
> +const size_t errmsg_len = 128;
> +char errmsg_buf[errmsg_len];
> +char *errmsg;
> +
> +for (; p != NULL; p = p->backingStore) {
> +if (virStorageSourceIsLocalStorage(p))
The above condition has a multiline body, so it must be enclosed in a
block.
> +if (unlink(p->path) < 0) {
I was considering whether we should use virFileRemove which will also
work properly on root-squashed NFS. I'm not sure though how easy it will
be to figure out the correct uid and gid inside this helper.
If you are interrested into investigating if it's possible, there should
be some prior art in the qemu driver where we try to get the uid/gid frm
virStorageSource if it's configured, then fall back to the domain
uid/gid of the process.
> +errmsg = strerror_r(errno, errmsg_buf, errmsg_len);
Please use g_strerror(); It does not require any of the buffers and
stuff.
> +VIR_WARN("Unable to remove snapshot image file %s (%s)",
> + p->path, errmsg);
> +}
> +}
The rest looks good.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/4] qemu: block: use the delete flag to delete snapshot images if requested
When blockcommit finishes successfully, one of the
qemuBlockJobProcessEventCompletedCommit() and
qemuBlockJobProcessEventCompletedActiveCommit() event handlers is called.
This is where the delete flag (stored in qemuBlockJobCommitData since the
previous commit) can actually be used to delete the committed snapshot
images if requested.
Signed-off-by: Pavel Mores
---
src/qemu/qemu_blockjob.c | 31 +++
1 file changed, 31 insertions(+)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 7d94a6ce38..cf221a2839 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -965,6 +965,31 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr
driver,
}
+/**
+ * Helper for qemuBlockJobProcessEventCompletedCommit() and
+ * qemuBlockJobProcessEventCompletedActiveCommit(). Relies on adjustments
+ * these functions perform on the 'backingStore' chain to function correctly.
+ *
+ * TODO look into removing backing store for non-local snapshots too
+ */
+static void
+qemuBlockJobUnlinkCommittedStorage(virStorageSourcePtr top)
+{
+virStorageSourcePtr p = top;
+const size_t errmsg_len = 128;
+char errmsg_buf[errmsg_len];
+char *errmsg;
+
+for (; p != NULL; p = p->backingStore) {
+if (virStorageSourceIsLocalStorage(p))
+if (unlink(p->path) < 0) {
+errmsg = strerror_r(errno, errmsg_buf, errmsg_len);
+VIR_WARN("Unable to remove snapshot image file %s (%s)",
+ p->path, errmsg);
+}
+}
+}
+
/**
* qemuBlockJobProcessEventCompletedCommit:
* @driver: qemu driver object
@@ -1031,6 +1056,9 @@ qemuBlockJobProcessEventCompletedCommit(virQEMUDriverPtr
driver,
job->data.commit.topparent->backingStore = job->data.commit.base;
qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob,
job->data.commit.top);
+
+if (job->data.commit.deleteCommittedImages)
+qemuBlockJobUnlinkCommittedStorage(job->data.commit.top);
virObjectUnref(job->data.commit.top);
job->data.commit.top = NULL;
@@ -1121,6 +1149,9 @@
qemuBlockJobProcessEventCompletedActiveCommit(virQEMUDriverPtr driver,
job->disk->src->readonly = job->data.commit.top->readonly;
qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob,
job->data.commit.top);
+
+if (job->data.commit.deleteCommittedImages)
+qemuBlockJobUnlinkCommittedStorage(job->data.commit.top);
virObjectUnref(job->data.commit.top);
job->data.commit.top = NULL;
/* the mirror element does not serve functional purpose for the commit job
*/
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
