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

Osier Yang jyang at redhat.com
Mon Feb 11 19:09:47 UTC 2013


On 2013年02月12日 01:44, Daniel P. Berrange wrote:
> 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.
>

Okay, I have no more reason to not go this way.

>
> 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.

Indeed, will add.

>
> Daniel




More information about the libvir-list mailing list