[Libvir] [PATCH] Allow modifying existing block device, making virDomainAttachDevice modify the device if it already exists
10:45 a.m.
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(a)redhat.com | virtualization library http://libvirt.org
10:59 a.m.
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
11:15 a.m.
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 -=|
1:37 p.m.
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(a)redhat.com | virtualization library http://libvirt.org
6:47 a.m.
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
6259
days inactive
6263
days old
4 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Hugh Brock