[v1] Basic implementation of persistent reservations

[libvirt] [PATCH 02/14] qemuDomainDiskChangeSupported: Deny changing reservations

Posted by Michal Privoznik 8 years ago

Couple of reasons for that:

a) there's no monitor command to change path where the pr-helper
connects to, or
b) there's no monitor command to introduce a new pr-helper for a
disk that already exists.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/libvirt_private.syms  |  1 +
 src/qemu/qemu_domain.c    |  8 ++++++++
 src/util/virstoragefile.c | 18 ++++++++++++++++++
 src/util/virstoragefile.h |  2 ++
 4 files changed, 29 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b0dc76b91..424f2ef64 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2719,6 +2719,7 @@ virStorageNetHostTransportTypeToString;
 virStorageNetProtocolTypeToString;
 virStoragePRDefFormat;
 virStoragePRDefFree;
+virStoragePRDefIsEqual;
 virStoragePRDefParseNode;
 virStorageSourceBackingStoreClear;
 virStorageSourceClear;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 441bf5935..e8539dcab 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6865,6 +6865,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
     CHECK_EQ(src->readonly, "readonly", true);
     CHECK_EQ(src->shared, "shared", true);
 
+    if (!virStoragePRDefIsEqual(disk->src->pr,
+                                orig_disk->src->pr)) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+                       _("cannot modify field '%s' of the disk"),
+                       "reservations");
+        return false;
+    }
+
 #undef CHECK_EQ
 
     return true;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index fda6617f7..469f7c97b 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2060,6 +2060,24 @@ virStoragePRDefFormat(virBufferPtr buf,
 }
 
 
+bool
+virStoragePRDefIsEqual(virStoragePRDefPtr a,
+                       virStoragePRDefPtr b)
+{
+    if (!a && !b)
+        return true;
+
+    if (!a || !b)
+        return false;
+
+    if (a->enabled != b->enabled ||
+        a->managed != b->managed ||
+        STRNEQ_NULLABLE(a->path, b->path))
+        return false;
+
+    return true;
+}
+
 virSecurityDeviceLabelDefPtr
 virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src,
                                     const char *model)
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index f82c20cf4..d07035d65 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -383,6 +383,8 @@ virStoragePRDefPtr virStoragePRDefParseNode(xmlDocPtr xml,
                                             xmlNodePtr root);
 void virStoragePRDefFormat(virBufferPtr buf,
                            virStoragePRDefPtr prd);
+bool virStoragePRDefIsEqual(virStoragePRDefPtr a,
+                            virStoragePRDefPtr b);
 
 virSecurityDeviceLabelDefPtr
 virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src,
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/14] qemuDomainDiskChangeSupported: Deny changing reservations

Posted by Peter Krempa 7 years, 12 months ago

On Thu, Jan 18, 2018 at 17:04:34 +0100, Michal Privoznik wrote:
> Couple of reasons for that:
> 
> a) there's no monitor command to change path where the pr-helper
> connects to, or
> b) there's no monitor command to introduce a new pr-helper for a
> disk that already exists.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/libvirt_private.syms  |  1 +
>  src/qemu/qemu_domain.c    |  8 ++++++++
>  src/util/virstoragefile.c | 18 ++++++++++++++++++
>  src/util/virstoragefile.h |  2 ++
>  4 files changed, 29 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index b0dc76b91..424f2ef64 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2719,6 +2719,7 @@ virStorageNetHostTransportTypeToString;
>  virStorageNetProtocolTypeToString;
>  virStoragePRDefFormat;
>  virStoragePRDefFree;
> +virStoragePRDefIsEqual;
>  virStoragePRDefParseNode;
>  virStorageSourceBackingStoreClear;
>  virStorageSourceClear;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 441bf5935..e8539dcab 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6865,6 +6865,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
>      CHECK_EQ(src->readonly, "readonly", true);
>      CHECK_EQ(src->shared, "shared", true);
>  
> +    if (!virStoragePRDefIsEqual(disk->src->pr,
> +                                orig_disk->src->pr)) {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +                       _("cannot modify field '%s' of the disk"),
> +                       "reservations");

Is there a particular reason to format 'reservations' separately?

Translations should not translate it, but doing this is generally deemed
translation unfriendly.

Rest looks good to me.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/14] qemuDomainDiskChangeSupported: Deny changing reservations

Posted by Andrea Bolognani 7 years, 12 months ago

On Mon, 2018-02-12 at 16:40 +0100, Peter Krempa wrote:
> > @@ -6865,6 +6865,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
> >      CHECK_EQ(src->readonly, "readonly", true);
> >      CHECK_EQ(src->shared, "shared", true);
> >  
> > +    if (!virStoragePRDefIsEqual(disk->src->pr,
> > +                                orig_disk->src->pr)) {
> > +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> > +                       _("cannot modify field '%s' of the disk"),
> > +                       "reservations");
> 
> Is there a particular reason to format 'reservations' separately?
> 
> Translations should not translate it, but doing this is generally deemed
> translation unfriendly.

Wouldn't that actually be better for translators? Assuming there's
going to be more than a single field, you can translate the message
once and it will work for all instances.

Of course that only works if, like in this case, the variable part
itself does not need translation.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/14] qemuDomainDiskChangeSupported: Deny changing reservations

Posted by Peter Krempa 7 years, 12 months ago

On Mon, Feb 12, 2018 at 17:19:24 +0100, Andrea Bolognani wrote:
> On Mon, 2018-02-12 at 16:40 +0100, Peter Krempa wrote:
> > > @@ -6865,6 +6865,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
> > >      CHECK_EQ(src->readonly, "readonly", true);
> > >      CHECK_EQ(src->shared, "shared", true);
> > >  
> > > +    if (!virStoragePRDefIsEqual(disk->src->pr,
> > > +                                orig_disk->src->pr)) {
> > > +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> > > +                       _("cannot modify field '%s' of the disk"),
> > > +                       "reservations");
> > 
> > Is there a particular reason to format 'reservations' separately?
> > 
> > Translations should not translate it, but doing this is generally deemed
> > translation unfriendly.
> 
> Wouldn't that actually be better for translators? Assuming there's
> going to be more than a single field, you can translate the message
> once and it will work for all instances.
> 
> Of course that only works if, like in this case, the variable part
> itself does not need translation.

Um yes, in this case it will actually help due to the surrounding code
using that pattern a lot and also tha the field should not be
translated.

I retract my comment.

ACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list