[libvirt] [PATCHv3 08/26] storage: Move readonly and shared flags to disk source from disk def

[Eric Blake]

On 06/25/2014 10:54 AM, Peter Krempa wrote:
> In the future we might need to track state of individual images. Move
> the readonly and shared flags to the virStorageSource struct so that we
> can keep them in a per-image basis.
> ---

My immediate reaction is that all backing files are generally readonly,
so when would we ever label them differently?  Then again, we
temporarily mark files readwrite during commit.

For shared, this move makes total sense.  (Shared is a host-only concept
- the file is read-only but must not be relabeled by libvirt because it
may be shared by other domains).  And for how we are using readonly
(label the host image differently than if it were read-write), it also
seems to make sense.  If we implemented reference-counted storage source
objects, the difference between shared and readonly is whether a second
reference can be obtained on a file already in use.

One thing is sitting a little uneasy on my mind - do we have (or need,
or want) a way to affect guest ABI by the readonly designation?  That
is, does it ever make sense to advertise to the guest that a disk is
readonly (maybe if presenting the guest a virtual DVD drive, the guest
will act differently if it is emulated as a DVD-ROM vs. if it is
emulated as a DVD-RW that can be burned)?  And if so, I could see a case
where we might want an image to be marked readonly to the guest
perspective, regardless of whether the host files are labeled for
readonly use.

But I've spend some time thinking about it, and can't come up with any
cases where having a readonly disk (guest point of view) would still
require a readwrite image from the host; and that tracking whether the
guest disk is readonly by deferring to whether the host source is
readonly seems to be reliable.

I also don't know if we will ever want to update our <backingChain> live
xml to expose whether backing chain elements are temporarily using a
read-write label, even though they default to readonly; or even letting
the user choose between <readonly/> vs. <shared/> for backing chain
elements.  This patch opens up some possibilities to think about for
future changes.

Okay, for all my ramblings above, I still can't articulate a firm reason
why this might be a bad idea, so I can live with it going in.

>  src/conf/domain_conf.c          | 18 ++++++++++--------
>  src/conf/domain_conf.h          |  2 --
>  src/libxl/libxl_conf.c          |  2 +-
>  src/locking/domain_lock.c       |  4 ++--
>  src/lxc/lxc_cgroup.c            |  2 +-
>  src/lxc/lxc_controller.c        |  2 +-
>  src/lxc/lxc_driver.c            |  2 +-
>  src/qemu/qemu_cgroup.c          |  4 ++--
>  src/qemu/qemu_command.c         | 14 +++++++-------
>  src/qemu/qemu_conf.c            |  4 ++--
>  src/qemu/qemu_driver.c          |  8 ++++----
>  src/qemu/qemu_migration.c       | 16 ++++++++++------
>  src/security/security_dac.c     |  2 +-
>  src/security/security_selinux.c |  6 +++---
>  src/security/virt-aa-helper.c   |  2 +-
>  src/util/virstoragefile.h       |  6 ++++++
>  src/vbox/vbox_tmpl.c            | 30 +++++++++++++++---------------
>  src/xenxs/xen_sxpr.c            | 10 +++++-----
>  src/xenxs/xen_xm.c              | 10 +++++-----
>  19 files changed, 77 insertions(+), 67 deletions(-)
> 

> +++ b/src/conf/domain_conf.c
> @@ -5549,9 +5549,9 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
>                      goto error;
>                  }
>              } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) {
> -                def->readonly = true;
> +                def->src->readonly = true;
>              } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) {
> -                def->shared = true;
> +                def->src->shared = true;
>              } else if (xmlStrEqual(cur->name, BAD_CAST "transient")) {
>                  def->transient = true;

Note that transient remains a per-guest disk item, not a per-host image
item.

> @@ -13390,7 +13390,8 @@ virDomainDiskDefCheckABIStability(virDomainDiskDefPtr src,
>          return false;
>      }
> 
> -    if (src->readonly != dst->readonly || src->shared != dst->shared) {
> +    if (src->src->readonly != dst->src->readonly ||
> +        src->src->shared != dst->src->shared) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                         _("Target disk access mode does not match source"));

You know, I think this ABI check is overly strict - a guest can't tell
the difference between whether a host image is <shared/> or <readonly/>
(the only difference between those two exclusive flags is whether other
domains may use the file at the same time).  But if we relax it, it
should be a separate patch.

ACK.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140625/effb2cf3/attachment-0001.sig>