Re: [libvirt] [PATCH 3/4] qemu: Don't remove hash entry of other domains

On Mon, Feb 11, 2013 at 07:37:14PM +0800, Osier Yang wrote: > On 2013年02月11日 18:59, Daniel P. Berrange wrote: >> On Fri, Feb 08, 2013 at 09:08:01PM +0800, Osier Yang wrote: >>> qemuProcessStart invokes qemuProcessStop when fails, to avoid >>> removing hash entry which belongs to other domain(s), this introduces >>> a new virBitmapPtr argument for qemuProcessStop. And qemuProcessStart >>> sets the bit for the disk only if it's successfully added into the >>> hash table. Thus if the argument is provided for qemuProcessStop, it >>> can't remove the hash entry belongs to other domain(s). >> >> I find this approach rather dubious - IMHO it is a sign that you're >> not recording enough information in the shared disk hash. eg perhaps >> the hash ought to be recording the UUID of each VM that is holding >> a reference. That way you're protected from qemuProcessStop() trying >> todo something wrong. >> > > I'm doubting about if it's really deserved to maintain a bunch of > arrays in the hash entry. As we only need the recording for > qemuProcessStart, it's much simpler to only use a bitmap to record > the added disks for a VM in qemuProcessStart from my p.o.v. IMHO it is a more robust design to record which VM is owning the disk, since it prevents us introducing errors elsewhere in the code which can lead to the same kind of problem later.

Incidentally, how does the shared disks hash get populated when libvirtd starts up& reconnects to existing running VMs ? AFAICT, nothing is being done in that codepath.

Daniel