Re: [libvirt] [PATCH v3 01/11] conf: Add some defined NFS Storage Pool mount options

On 1/16/19 7:00 AM, Daniel P. Berrangé wrote: In v1 it seems to me at least the objection was arbitrarily named mount_opts, so specifically named mount_opts felt like a fair trade-off. In v2 you noted the features element to be similar to the domain features, but then pointed out that you felt we could just default to using them. A followup pointed out one concern over OS syntax differences, which is essentially why I took this approach - force someone to provide the option. If that option used different syntax on some other OS, then someone else who cares about that OS could send a patch to change the name generated (and we avoid making any assumptions). True we could, but believe it or not readonly is not even defined in storagepool.rng. It's a domaincommon.rng to be used for hypervisor options. Adding ro to the mount_opts list was essentially a shortcut of patches to include ro with the others. I saw nfsvers a bit different since it would provide more than a boolean option. Still beyond NFS I'm trying to envision how we'd supply readonly such that it would matter for the pool. Wouldn't networked pools require the server side to have set up readonly? For local pools how much of the exposure of the volume would be controllable such that the pool could manage the readonly-ness? A virDomainDiskTranslateSourcePool "option" I suppose that could be associated with the domain readonly attribute. Perhaps ro or readonly should just be separate then because it involves a lot more interaction. Does the overhead of code addition outweigh the usefulness of a generic pool readonly option? No one's asked for such a feature, so sneaking in ro to/for an NFS mount option felt reasonable. Again, the fear of doing that is some OS has different syntax (from your v2 response it's possible that FreeBSD mount command options may differ from Linux mount command options). So, using named options seemed to be a better tradeoff than just altering the command generation to add those options for NFS mounts. I'm not deadset against altering the defaults we set. It's certainly the easiest approach. Drop patch 1 and 3... Changes patch 2 to a very static operation, changing the existing .args output for NFS pools, and still leaves patches 4-11 mostly unchanged other than to "merge" the changes from patch 2. I really don't see the point of differently named XML. As much as you don't seem to like mount_opts I'm not a fan of features. At least with mount_opts it's obvious what the use is. I just don't see mount options as features in the same light as hypervisor features. It's a command option not a feature for the pool. Other pools use specifically named elements to support their specific features/needs <name>, <host>, <adapter>, <auth>, <initiator>, etc. But something that is very specific to how the NFS is being used is required to use a new, generic "features" element. If by defaulting to nosuid, nodev, and noexec we run into trouble with some other OS, then that requires every pool on that OS to be modified to add these because we changed the default. That seems counter to other examples where we force to opt-in. Although because these are security related, I can see an argument for defaulting to them other than of course that NFS doesn't default that way. Rock and hard place. FWIW: AIUI, the default for the listed option options is the inverse. That is "suid" is the default for NFS... Likewise, "dev", "exec", and "rw". So supplying "suid" instead of "nosuid" doesn't mean anything since "suid" is the default. It does mean though code to "remove" default options which in a way is no different than code to supply an option that isn't default. Restated - knowing that "suid" is the default, we don't supply it. Of course we could, but that hasn't been the norm thus far. We just take the NFS defaults. Still if this were to follow the domain model, then why wouldn't it be: <features> <netfs> <option name='suid' state='{on|off}'/> <option name='exec' state='{on|off}'/> ... (or <start_option .../> since this isn't create, define, build, destroy, or undefine) I would think that would be far more confusing, but seems to me to follow how features are assigned/used for specific hypervisors. It still requires some sort of mapping of names and we're not allowing arbitrary ones. Does nosuid, noexec, or nodev have meaning beyond that of the meaning used for NFS? If the answer is no, then it's a very specific feature to a very specific pool. And what "features" would we consider allowing without having to craft a different backend? Allowing some provided option for LVM commands could result in needing to write code that would be able to handle multiple different formats. In particular I'm thinking about how thin provisioning is supported. Very different way to configure and a completely different means to scan the generated output looking for usable volumes. Previous attempts to co-mingle in the current logical backend were NACK'd and no one has taken up the create a new backend that supports thin pools vs thin snapshots (I think the latter is the terminology used, it's been a few years). For iSCSI we now have the iscsi-direct backend which perhaps could make use of something, but it'd be something fairly specific to some iSCSI feature which is no different (to me at least) than providing some specific option for netfs. John