Re: [libvirt] [PATCH v2] qemu: Check for existence of provided *_tls_x509_cert_dir

On 07/19/2017 09:56 AM, Ján Tomko wrote: Noted, point taken and I'll drop this hunk. One would think/assume, but then again the point of the bz was about the comments being too vague, so I've taken the opposite approach to be somewhat overly verbose. If the directory exists, but doesn't contain the right files, libvirt startup will succeed. However, any QEMU domain that's started *and* uses TLS in some way will fail when QEMU starts if the right files don't exist. The libvirt config code doesn't check that the specific files exist in the directory. It's perhaps "notable" that /etc/pki/qemu is never checked for existence. So if things were commented out and someone tried to use TLS without first creating an /etc/pki/qemu, then domain startup would fail if it was configured to use TLS (or used for migration). That's a quirk in the "default" setting because we don't provide the /etc/pki/qemu directory. Again, since the bz indicated it wasn't clear, I was trying be overly obvious. Sometimes being overly succinct in documentation has advantages with respect to setting expectations. How about if I change them to : # In order to overwrite the default path alter the following. This path # definition will be used as the default path for other *_tls_x509_cert_dir # configuration settings if they are not specifically set. (and assuming the changes descibed in [1] below) But that doesn't reflect the actuality of the code. So are you expecting the code to change too? During virQEMUDriverConfigNew (SET_TLS_X509_CERT_DEFAULT), if the "/etc/pki/libvirt-*" doesn't exist (where * would be vnc, spice, chardev, migrate), then by default it is set to /etc/pki/qemu. If someone then later changes default_tls_x509_cert_dir, then that directory is used instead of the default /etc/pki/qemu. If the other settings remain commented out, then their defaults are /etc/pki/qemu. That being said - the virQEMUDriverConfigSetCertDir could change to add code that could check if "default" was set to something other than the default, then copy in "default" (and assume it was already checked) [1]. Strange snipping seems to have missed a [...] since the patch definitely had more here, but they're all the same... [1] (from above comments)... If the entry was commented out, then if cfg->defaultTLSx509certdir is not /etc/pki/qemu (the default), then STRDUP into *value: if (!tlsCertDir) { if (STRNEQ_NULLABLE(defaultTLS, *value)) { if (VIR_STRDUP(*value, defaultTLS) < 0) return -1 } return 0; } where defaultTLS is either cfg->defaultTLSx509certdir or NULL for default. Since for the default case, the STRDUP isn't necessary; however, for others (vnc, spice, chardev, and migrate) the *value would be set to whatever cfg->defaultTLSx509certdir is. If this happens, then the keeping the original comment is fine and the bz would change it's "expected results" perhaps although it isn't clear because it's not "default" that's changing. And your suggestion for a better error is what? Should I create a new one? Should I use virReportSystemError(ENOENT, ???)? IDC really, but please don't make me guess! So this is different in what way than securityDriverNames, controllers, migrateHost, migrationAddress, or namespaces? I think creating some new validation routine to be run afterwards is outside the scope of what's being done here especially considering there already is validation taking place. John