[Libvir] [PATCH] Allow modifying existing block device, making virDomainAttachDevice modify the device if it already exists
The attached patch adds code to xend_internal.c:xenDomainAttachDevice that checks to see if the device mentioned in the passed-in XML already exists, and if so calls op_device_configure to modify the device. It is particularly useful for connecting and disconnecting a hardware cdrom device to an FV guest. I considered making new API a la virDomainModifyDevice, but decided overloading virDomainAttachDevice was cleaner (and didn't change existing API). I have tested the patch on f7 with xen 3.1 and a windows 2000 guest. Since the patch merely wraps Xen's device_configure call, we're not adding much risk of breakage. Please ACK. Take care, --Hugh -- Red Hat Virtualization Group http://redhat.com/virtualization Hugh Brock | virt-manager http://virt-manager.org hbrock@redhat.com | virtualization library http://libvirt.org
On Thu, Sep 06, 2007 at 11:45:39AM -0400, Hugh Brock wrote:
The attached patch adds code to xend_internal.c:xenDomainAttachDevice that checks to see if the device mentioned in the passed-in XML already exists, and if so calls op_device_configure to modify the device. It is particularly useful for connecting and disconnecting a hardware cdrom device to an FV guest.
I considered making new API a la virDomainModifyDevice, but decided overloading virDomainAttachDevice was cleaner (and didn't change existing API).
I have tested the patch on f7 with xen 3.1 and a windows 2000 guest. Since the patch merely wraps Xen's device_configure call, we're not adding much risk of breakage.
Okay looks sensible.
diff -ruN libvirt.orig/src/xend_internal.c libvirt/src/xend_internal.c --- libvirt.orig/src/xend_internal.c 2007-09-06 11:28:05.000000000 -0400 +++ libvirt/src/xend_internal.c 2007-09-06 10:47:25.000000000 -0400 @@ -3087,6 +3087,7 @@ char *sexpr, *conf, *str; int hvm = 0, ret; xenUnifiedPrivatePtr priv; + char class[8], ref[80];
if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) { virXendError((domain ? domain->conn : NULL), VIR_ERR_INVALID_ARG, @@ -3116,8 +3117,16 @@ *(conf + strlen(conf) -1) = 0; /* suppress final ) */ } else conf = sexpr; - ret = xend_op(domain->conn, domain->name, "op", "device_create", - "config", conf, NULL); + if (virDomainXMLDevID(domain, xml, class, ref)) { + /* device doesn't exist, define it */ + ret = xend_op(domain->conn, domain->name, "op", "device_create", + "config", conf, NULL); + } + else { + /* device exists, attempt to modify it */ + ret = xend_op(domain->conn, domain->name, "op", "device_configure", + "config", conf, "dev", ref, NULL); + } free(sexpr); return ret; }
virDomainXMLDevID looks frightening to me since we write to an array of undisclosed size, but that is independant from the patch which looks fine to me. +1 Thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Thu, Sep 06, 2007 at 11:59:08AM -0400, Daniel Veillard wrote:
On Thu, Sep 06, 2007 at 11:45:39AM -0400, Hugh Brock wrote:
The attached patch adds code to xend_internal.c:xenDomainAttachDevice that checks to see if the device mentioned in the passed-in XML already exists, and if so calls op_device_configure to modify the device. It is particularly useful for connecting and disconnecting a hardware cdrom device to an FV guest.
I considered making new API a la virDomainModifyDevice, but decided overloading virDomainAttachDevice was cleaner (and didn't change existing API).
I have tested the patch on f7 with xen 3.1 and a windows 2000 guest. Since the patch merely wraps Xen's device_configure call, we're not adding much risk of breakage.
Okay looks sensible.
diff -ruN libvirt.orig/src/xend_internal.c libvirt/src/xend_internal.c --- libvirt.orig/src/xend_internal.c 2007-09-06 11:28:05.000000000 -0400 +++ libvirt/src/xend_internal.c 2007-09-06 10:47:25.000000000 -0400 @@ -3087,6 +3087,7 @@ char *sexpr, *conf, *str; int hvm = 0, ret; xenUnifiedPrivatePtr priv; + char class[8], ref[80];
if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) { virXendError((domain ? domain->conn : NULL), VIR_ERR_INVALID_ARG, @@ -3116,8 +3117,16 @@ *(conf + strlen(conf) -1) = 0; /* suppress final ) */ } else conf = sexpr; - ret = xend_op(domain->conn, domain->name, "op", "device_create", - "config", conf, NULL); + if (virDomainXMLDevID(domain, xml, class, ref)) { + /* device doesn't exist, define it */ + ret = xend_op(domain->conn, domain->name, "op", "device_create", + "config", conf, NULL); + } + else { + /* device exists, attempt to modify it */ + ret = xend_op(domain->conn, domain->name, "op", "device_configure", + "config", conf, "dev", ref, NULL); + } free(sexpr); return ret; }
virDomainXMLDevID looks frightening to me since we write to an array of undisclosed size, but that is independant from the patch which looks fine to me.
Yep, this patch is fine, but we absolutely have to fix virDomainXMLDevID as the value it writes into the pre-allocated 'ref' parameter is taken from an XML attribute which can be an arbitrary size & can thus potentially overflow the '80' char buffer passed from xenDaemonDetachDevice or this new call for changing CD media. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Daniel P. Berrange wrote:
On Thu, Sep 06, 2007 at 11:59:08AM -0400, Daniel Veillard wrote:
On Thu, Sep 06, 2007 at 11:45:39AM -0400, Hugh Brock wrote:
The attached patch adds code to xend_internal.c:xenDomainAttachDevice that checks to see if the device mentioned in the passed-in XML already exists, and if so calls op_device_configure to modify the device. It is particularly useful for connecting and disconnecting a hardware cdrom device to an FV guest.
virDomainXMLDevID looks frightening to me since we write to an array of undisclosed size, but that is independant from the patch which looks fine to me.
Yep, this patch is fine, but we absolutely have to fix virDomainXMLDevID as the value it writes into the pre-allocated 'ref' parameter is taken from an XML attribute which can be an arbitrary size & can thus potentially overflow the '80' char buffer passed from xenDaemonDetachDevice or this new call for changing CD media.
Dan.
Very good -- if someone would apply this one please, I will have a look at making the XMLDevID code a bit saner... --Hugh -- Red Hat Virtualization Group http://redhat.com/virtualization Hugh Brock | virt-manager http://virt-manager.org hbrock@redhat.com | virtualization library http://libvirt.org
On Thu, Sep 06, 2007 at 02:37:50PM -0400, Hugh Brock wrote:
Daniel P. Berrange wrote:
On Thu, Sep 06, 2007 at 11:59:08AM -0400, Daniel Veillard wrote:
On Thu, Sep 06, 2007 at 11:45:39AM -0400, Hugh Brock wrote:
The attached patch adds code to xend_internal.c:xenDomainAttachDevice that checks to see if the device mentioned in the passed-in XML already exists, and if so calls op_device_configure to modify the device. It is particularly useful for connecting and disconnecting a hardware cdrom device to an FV guest.
virDomainXMLDevID looks frightening to me since we write to an array of undisclosed size, but that is independant from the patch which looks fine to me.
Yep, this patch is fine, but we absolutely have to fix virDomainXMLDevID as the value it writes into the pre-allocated 'ref' parameter is taken from an XML attribute which can be an arbitrary size & can thus potentially overflow the '80' char buffer passed from xenDaemonDetachDevice or this new call for changing CD media.
Dan.
Very good -- if someone would apply this one please, I will have a look at making the XMLDevID code a bit saner...
Okay, done, thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Hugh Brock