Re: [libvirt] [PATCH 01/14] virstoragefile: Introduce virStoragePRDef

On 01/29/2018 08:03 AM, Michal Privoznik wrote: Just trying to cover all the bases... Disks, Hostdevs, Storage Pools, SCSI, iSCSI, NPIV, etc. A few different ways to figure LUNs. I think you're right regarding the privileged command - it's what I have a recollection of when dealing with PR before with the rawio and sgio properties. I have access to the NPIV environment I use and can let you use it as well... So that part is not a problem. IIRC, this was a two pronged question, but I neglected to go back and be more explicit.. The second half of the question is when using a storage pool for your LUN, then how is the PR added? The <reservations> are only being processed from one very specific disk source type. For reference, see how many times encryption is added to various RNG definitions - including diskSourceVolume which is what I think you'd want to process a 'srcpool' definition. The virStorageTranslateDiskSourcePool handles the magic of generating the disk like structure used when building the command line. There's also a 'mode' attribute where if set to 'host' for an iSCSI LUN's will result in using the "local" filesystem path to the LUN rather than going through the iSCSI server. I would think for this case, then one would have to have the PR set up (if configured of course). Well that'll make life a bit easier. IIRC, this creates a /dev/sgN on the guest. I know the design was agreed upon previously, but nothing like actual patches to open Pandora's Box again ;-). I might be off in the weeds here, but taking a step back and typing thoughts... If MHP is a QEMU related process - IOW, something QEMU creates, then why is it libvirt's responsibility to manage it? Why would/should libvirt be responsible for restarting it if it goes away? The death would be noticeable if we were notified that the socket closed or perhaps if QEMU sent an event when it noticed. If the daemon was having a problem, then wouldn't restarting it possibly cause a never ending fail/restart cycle? What/Who stops that? Perhaps the UHP model is how we should be treating things - that is "mostly" not our problem. Shouldn't the extent of libvirt's "management" be nothing more than using it and handling when the connection dies by some action defined for the device that's using it (e.g. domain shutdown, domain pause, device hot unplug)?. Subsequent hot-plug's would need to determine that the connection to the daemon can be made; otherwise, the attempt fails. That may need to be something checked after the device add. Defining the close action on a device by device basis allows the domain configurer to make a choice based on whether it's a boot/system/important disk/lun or just something being used for something. Another random, but related thought... QEMU has made this change to add this PR daemon to manage something and communicate with libvirt by some defined socket... Someone else says, great idea, let's copy that and libvirt will be able to manage it. After all imitation is a form of flattery. That leaves libvirt trying to fit this new thing into the existing model - either by liberally copying the same code and changing the names to protect the innocent or refactoring to generate common paths for separate technologies. Still in the long run what we have for the current mechanism is a socket by path. So if we consider that the common point why not have a way to define socket paths to external daemon's by path. IOW: Trying to make this generic enough that we can just have a "type" that would be able to label the type of socket (e.g. "PR") <disk>... <source>.. <socket type='unix' path='/path/to/socket.sock' mode='client|server' closeaction='shutdown|pause|unplug' [daemon='/path/to/daemon' args='list of daemon args']>NAME</socket> Where : optional daemon & args will be used to spin out a new daemon NAME is just some string, but must be UNIQUE amongs other sockets (we could store these in a hash table any number of sockets) The thought behind 'daemon' attribute is to avoid the qemu.conf changes... The args allows defining a way to start the daemon debug or however it's desired - it's just a string copied/passed to the daemon. I would think it's up to daemon code to make all the rules and manage everything including security, cgroups, acls, etc. etc. We don't care if it's a LUN, a Disk, an LV, a Block device, etc. If the daemon wants to somehow support migration, then have at it. We probably should be as hands off as possible. OK - rambled on long enough ;-) John FWIW: Coverity issues found in current series... Patch 9: * In qemuProcessSetupOnePRDaemon, @pidfile is NULL if jump to cleanup if prHelperName is not executable causing a unlink(NULL) which is bad. Also the @cfg is leaked. Patch 10: * In qemuDomainObjPrivateXMLParsePRs, virXPathNodeSet can return a -1 into an unsigned @n value, thus the subsequent for loop will take a while.