[libvirt] [PATCH] add the check whether the domain has already used a disk which attach
Hi, "attach-disk" does not give an error, when the domain which is attached has already used the source which attach. This has danger of the data corruption. I make the patch to add the check of this. This patch outputs an error, when the domain which is attached has already used the source which attach. Thanks, Shigeki Sakamoto. Index: src/virsh.c =================================================================== RCS file: /data/cvs/libvirt/src/virsh.c,v retrieving revision 1.170 diff -u -p -r1.170 virsh.c --- src/virsh.c 13 Oct 2008 16:46:29 -0000 1.170 +++ src/virsh.c 4 Nov 2008 07:28:20 -0000 @@ -4993,6 +4993,11 @@ cmdAttachDisk(vshControl *ctl, const vsh char *source, *target, *driver, *subdriver, *type, *mode; int isFile = 0, ret = FALSE; char *buf = NULL, *tmp = NULL; + xmlDocPtr xml = NULL; + xmlXPathObjectPtr obj = NULL; + xmlXPathContextPtr ctxt = NULL; + char *doc; + char xpath[PATH_MAX]; if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) goto cleanup; @@ -5034,6 +5039,35 @@ cmdAttachDisk(vshControl *ctl, const vsh } } + if (source) { + doc = virDomainGetXMLDesc(dom, 0); + if (!doc) + goto cleanup; + + xml = xmlReadDoc((const xmlChar *) doc, "domain.xml", NULL, + XML_PARSE_NOENT | XML_PARSE_NONET | + XML_PARSE_NOWARNING); + free(doc); + if (!xml) + goto cleanup; + ctxt = xmlXPathNewContext(xml); + if (!ctxt) + goto cleanup; + + sprintf(xpath, "string(/domain/devices/disk/source[@dev='%s']/@dev)", source); + obj = xmlXPathEval(BAD_CAST xpath, ctxt); + if (!strcmp(obj->stringval, source)) { + vshError(ctl, FALSE, _("Disk %s is already in use by own guest"), source); + goto cleanup; + } + sprintf(xpath, "string(/domain/devices/disk/source[@file='%s']/@file)", source); + obj = xmlXPathEval(BAD_CAST xpath, ctxt); + if (!strcmp(obj->stringval, source)) { + vshError(ctl, FALSE, _("Disk %s is already in use by own guest"), source); + goto cleanup; + } + } + /* Make XML of disk */ tmp = vshMalloc(ctl, 1); if (!tmp) goto cleanup; @@ -5123,6 +5157,10 @@ cmdAttachDisk(vshControl *ctl, const vsh ret = TRUE; cleanup: + xmlXPathFreeObject(obj); + xmlXPathFreeContext(ctxt); + if (xml) + xmlFreeDoc(xml); if (dom) virDomainFree(dom); free(buf);
On Wed, Nov 05, 2008 at 10:05:14AM +0900, S.Sakamoto wrote:
Hi,
"attach-disk" does not give an error, when the domain which is attached has already used the source which attach. This has danger of the data corruption.
I make the patch to add the check of this. This patch outputs an error, when the domain which is attached has already used the source which attach.
What hypervisor were you testing with ? AFAIR, XenD should already raise an appropriate error in this scenario. The libvirt QEMU driver, however, does need this checking. That should really be done in the qemu_driver.c attach method. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, Nov 05, 2008 at 10:05:14AM +0900, S.Sakamoto wrote:
Hi,
"attach-disk" does not give an error, when the domain which is attached has already used the source which attach. This has danger of the data corruption.
I make the patch to add the check of this. This patch outputs an error, when the domain which is attached has already used the source which attach.
What hypervisor were you testing with ? AFAIR, XenD should already raise an appropriate error in this scenario.
XenD Don't check this when I confirm it in xen(cs 18756:5fd51e1e9c79). Therefore, I think that it had better add check to virsh.c, because this should be checked whether xen or qemu. (Like giving an error when "virsh start" is performed to a starting domain) Thanks, Shigeki Sakamoto.
On Thu, Nov 06, 2008 at 07:15:07PM +0900, S.Sakamoto wrote:
On Wed, Nov 05, 2008 at 10:05:14AM +0900, S.Sakamoto wrote:
Hi,
"attach-disk" does not give an error, when the domain which is attached has already used the source which attach. This has danger of the data corruption.
I make the patch to add the check of this. This patch outputs an error, when the domain which is attached has already used the source which attach.
What hypervisor were you testing with ? AFAIR, XenD should already raise an appropriate error in this scenario.
XenD Don't check this when I confirm it in xen(cs 18756:5fd51e1e9c79).
Therefore, I think that it had better add check to virsh.c, because this should be checked whether xen or qemu.
No, because then the check is only performed for people using virsh, and would have to be duplicated across every single other application using libvirt. If it is done in the XenD or libvirtd at time of use, then all users of libvirt are automatically protected. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"attach-disk" does not give an error, when the domain which is attached has already used the source which attach. This has danger of the data corruption.
I make the patch to add the check of this. This patch outputs an error, when the domain which is attached has already used the source which attach.
What hypervisor were you testing with ? AFAIR, XenD should already raise an appropriate error in this scenario.
XenD Don't check this when I confirm it in xen(cs 18756:5fd51e1e9c79).
Therefore, I think that it had better add check to virsh.c, because this should be checked whether xen or qemu.
No, because then the check is only performed for people using virsh, and would have to be duplicated across every single other application using libvirt. If it is done in the XenD or libvirtd at time of use, then all users of libvirt are automatically protected. Of course, it is the best if it guarded in Xend. But, the user may be not protected depending on cs of xen,
Had better it attach check-method in libvirt.c to protect all users of libvirt ? Thanks, Shigeki Sakamoto.
On Mon, Nov 10, 2008 at 07:38:58PM +0900, S.Sakamoto wrote:
"attach-disk" does not give an error, when the domain which is attached has already used the source which attach. This has danger of the data corruption.
I make the patch to add the check of this. This patch outputs an error, when the domain which is attached has already used the source which attach.
What hypervisor were you testing with ? AFAIR, XenD should already raise an appropriate error in this scenario.
XenD Don't check this when I confirm it in xen(cs 18756:5fd51e1e9c79).
Therefore, I think that it had better add check to virsh.c, because this should be checked whether xen or qemu.
No, because then the check is only performed for people using virsh, and would have to be duplicated across every single other application using libvirt. If it is done in the XenD or libvirtd at time of use, then all users of libvirt are automatically protected. Of course, it is the best if it guarded in Xend. But, the user may be not protected depending on cs of xen,
Had better it attach check-method in libvirt.c to protect all users of libvirt ?
yes the patch need to be done in libvirt.c to protect applications which are not virsh, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
"attach-disk" does not give an error, when the domain which is attached has already used the source which attach. This has danger of the data corruption.
I make the patch to add the check of this. This patch outputs an error, when the domain which is attached has already used the source which attach.
What hypervisor were you testing with ? AFAIR, XenD should already raise an appropriate error in this scenario.
XenD Don't check this when I confirm it in xen(cs 18756:5fd51e1e9c79).
Therefore, I think that it had better add check to virsh.c, because this should be checked whether xen or qemu.
No, because then the check is only performed for people using virsh, and would have to be duplicated across every single other application using libvirt. If it is done in the XenD or libvirtd at time of use, then all users of libvirt are automatically protected. Of course, it is the best if it guarded in Xend. But, the user may be not protected depending on cs of xen,
Had better it attach check-method in libvirt.c to protect all users of libvirt ?
yes the patch need to be done in libvirt.c to protect applications which are not virsh,
Of course, it is the best if it guarded in Xend. But, the user may be not protected depending on cs of xen,
Had better it attach check-method in libvirt.c to protect all users of libvirt ?
yes the patch need to be done in libvirt.c to protect applications which are not virsh,
I remake a patch so that user is protected in libvirt.c Thanks, Shigeki Sakamoto.
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
S.Sakamoto