[libvirt] [PATCH] blockcopy: check dst = identical device

[Chun Yan Liu]

>>> On 7/29/2014 at 11:47 PM, in message <53D7C1F6.4030705 at redhat.com>, Eric Blake
<eblake at redhat.com> wrote: 
> On 07/29/2014 07:37 AM, Eric Blake wrote: 
> > On 07/29/2014 03:59 AM, Chunyan Liu wrote: 
> >> Check whether dst is the same device as source, if yes, report 
> >> error and exit. 
> >> 
> >> Currently if dst is the same device as source, blockcopy is still 
> >> going and qemu 'drive-mirror' is executed. Considering that: 
> >> a). blockcopy to the same device is meaningless. b.) result is 
> >> unexpected. (tested with block device whose source path is /dev/sdaX, 
> >> after blockcopy, shutdown VM and then create VM from xml again, the 
> >> VM cannot be started.) This case should not be allowed. 
> >> 
> >> Signed-off-by: Chunyan Liu <cyliu at suse.com> 
> >> --- 
> >>  src/qemu/qemu_driver.c | 7 +++++++ 
> >>  1 file changed, 7 insertions(+) 
> >> 
> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c 
> >> index 704ba39..87a3790 100644 
> >> --- a/src/qemu/qemu_driver.c 
> >> +++ b/src/qemu/qemu_driver.c 
> >> @@ -15309,6 +15309,13 @@ qemuDomainBlockCopy(virDomainObjPtr vm, 
> >>      } 
> >>   
> >>      /* Prepare the destination file.  */ 
> >> +    if (STREQ(disk->src->path, dest)) { 
> >  
> > STREQ is too weak (consider symlinks, or even "a/b" vs. "a//b").  It 
> > will catch some cases, but not all. 
> >  
> > I don't know that we can reliably detect all possible ways the user can 
> > shoot themselves in the foot, so I'm not sure this patch is a good idea. 
>  
> A better idea would be to rely on the volume lease manager - obtaining a 
> lease should be impossible for an image already in use (and should even 
> cover the case of copying 'base <- active' onto 'base', which your 
> equality test wouldn't catch). I'm not sure why the lease manager is not 
> already flagging this issue - are we still using the nop lease manager 
> by default, and would the fcntl or sanlock lease manager do a better job? 

According to code, it's still using 'nop' by default.
Code:
    if (!(qemu_driver->lockManager =
          virLockManagerPluginNew(cfg->lockManagerName ?
                                  cfg->lockManagerName : "nop",
                                  "qemu",
                                  cfg->configBaseDir,
                                  0)))
        goto error;

Saw Daniel's virlockd patch series, the last patch:
"Change the default QEMU lock manager to 'lockd'" is ACKed. But this patch is
not committed. All other patches in that series have been committed.
http://www.redhat.com/archives/libvir-list/2012-December/msg00734.html

>  
> --  
> Eric Blake   eblake redhat com    +1-919-301-3266 
> Libvirt virtualization library http://libvirt.org 
>  
>