Re: [libvirt] PATCH] Stop double free
Mark Hamzy wrote: The stack trace is as follows: Program received signal SIGABRT, Aborted. 0x0035ad830265 in raise () from /lib64/libc.so.6 (gdb) bt #0 0x0035ad830265 in raise () from /lib64/libc.so.6 #1 0x0035ad831d10 in abort () from /lib64/libc.so.6 #2 0x0035ad86a84b in __libc_message () from /lib64/libc.so.6 #3 0x0035ad8722ef in _int_free () from /lib64/libc.so.6 #4 0x0035ad87273b in free () from /lib64/libc.so.6 #5 0x00406771 in vshDeinit (ctl=0x7fffd35d35e0) at virsh.c:8244 #6 0x004069a5 in vshError (ctl=0x7fffd35d35e0, doexit=value optimized out, format=0x414f66 %s) at virsh.c:7861 #7 0x004067c4 in vshDeinit (ctl=0x7fffd35d35e0) at virsh.c:8248 #8 0x0041335e in main (argc=3, argv=0x7fffd35d3748) at virsh.c:8493 I am trying to run libvirt-0.7.1-0.1.git3ef2e05.fc12.src.rpm on RHEL5.4. vshDeinit gets called twice, so ctl-name is freed twice. How about this patch then? Ah, I see now. Your patch is a workaround. The real problem is that vshDeinit is re-entering itself through: vshDeinit()-vshError()-vshDeinit() While your patch would fix the problem, I'm not sure it's a good long-term solution. Other differences might come up in the future, and trying to worry about vshDeinit being re-entrant is probably not worth the effort. (Indeed, it looks like there were earlier attempts to avoid this, but things have changed since then, breaking the workaround). I think we should make it so that vshDeinit() does not try to re-enter itself. At the moment I don't have a patch, but I would look at either splitting vshError() into vshPrintError() and vshError(), or just doing a couple of fprintf()'s directly in vshDeinit() and not calling vshError() at all (with a comment explaining why). -- Chris Lalancette -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/11] Generic data stream handling
On Mon, Aug 24, 2009 at 09:51:03PM +0100, Daniel P. Berrange wrote: The following series of patches introduce support for generic data streams in the libvirt API, the remote protocol, client daemon. The public API takes the form of a new object virStreamPtr and methods to read/write/close it The remote protocol was the main hard bit. Since the protocol allows multiple concurrent API calls on a single connection, this needed to also allow concurrent data streams. It is also not acceptable for a large data stream to block other traffic while it is transferring. Thus, we introduce a new protocol message type 'REMOTE_STREAM' to handle transfer for the stream data. A method involving a data streams starts off in the normal way with a REMOTE_CALL to the server, and a REMOTE_REPLY response message. If this was successful, there now follows the data stream traffic. For outgoing streams (data from client to server), the client will send zero or more REMOTE_STREAM packets containing the data with status == REMOTE_CONTINUE. These are asynchronous and not acknowledged by the server. At any time the server may send an async message with a type of REMOTE_STREAM and status of REMOTE_ERROR. This indicates to the client that the transfer is aborting at server request. If the client wishes to abort, it can send the server a REMOTE_STREAM+REMOTE_ERROR message. If the client finishes its data transfer, it will send a final REMOTE_STREAM+REMOTE_OK message, and the server will respond with the same. This full roundtrip handshake ensures any async error messages are guarenteed to be flushed For incoming data streams (data from server to client), the server sends zero or more REMOTE_STREAM packets containing the data with status == REMOTE_CONTINUE. These are asynchronous and not acknowledged by the client. At any time the client may send an async message with a type of REMOTE_STREAM and status of REMOTE_ERROR. This indicates to the server that the transfer is aborting at client request. If the server wishes to abort, it can send the server a REMOTE_STREAM+REMOTE_ERROR message. When the server finishes its data transfer, it will send a final REMOTE_STREAM+REMOTE_CONTINUE message ewith a data length of zero (ie EOF). The client will then send a REMOTE_STREAM+REMOTE_OK packet and the server will respond with the same. This full roundtrip handshake ensures any async error messages are guarenteed to be flushed This all ensures that multiple data streams can be active in parallel, and with a maximum data packet size of 256 KB, no single stream can cause too much latency on the connection for other API calls/streams. Okay, this is very similar in principle with HTTP pipelining with IMHO the same benefits and the same potential drawbacks. A couple of things to check might be: - the maximum amount of concurrent active streams allowed, for example suppose you want to migrate in emergency all the domains out of a failing machine, some level of serialization may be better than say attempting to migrate all 100 domains at the same time. 10 parallel stream might be better, but we need to make sure the API allows to report such condition. - the maximum chunking size, but with 256k I think this is covered. - synchronization internally between threads to avoid deadlocks or poor performances, that can be very hard to debug, so I guess an effort should be provided to explain how things are designed internally. But this sounds fine in general. The only thing it does not allow for is one API method to use two or more streams. These may be famous last words, but I don't think that use case will be neccessary for any of our APIs... as long as the limitation is documented especially in the parts of teh code where the assumption is made, sounds fine. The last 5 patches with a subject of [DEMO] are *NOT* intended to be committed to the repository. They merely demonstrate the use of data streams for a couple of hypothetical file upload and download APIs. Actually they were mostly to allow me to test the code streams code without messing around with the QEMU migration code. The immediate use case for this data stream code is Chris' QEMU migration patchset. The next use case is to allow serial console access to be tunnelled over libvirtd, eg to make 'virsh console GUEST' work remotely. This use case is why I included the support for non-blocking data streams and event loop integration (not required for Chris' migration use case) Okay, next to individual patches reviews, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Virsh migrate --live fails
Subba Rao, Sandeep M (STSD) wrote: Hi, I'm trying to live move a domU using virsh migrate command: [r...@rhel53xen1 ~]# xm list Name ID Mem(MiB) VCPUs State Time(s) Copy_RHELVM3 1 511 1 -b898.3 Domain-0 0 1507 4 r- 133702.5 [r...@rhel53xen1 ~]# virsh migrate --live Copy_RHELVM3 xen+ssh://15.154.102.20 error: server closed connection Often this is because libvirtd crashed. After this command, check that libvirtd is still running, and if not, you might want to collect the core dump from it and get the output of thread apply all bt. [r...@rhel53xen1 ~]# However the same works using xm migrate --live. The source and the destination are RHEL 5.3 Xen hosts. Libvirt: [r...@rhel53xen1 ~]# rpm -qa | grep libvirt libvirt-0.3.3-14.el5 libvirt-devel-0.3.3-14.el5 libvirt-cim-0.5.1-4.el5 libvirt-python-0.3.3-14.el5 At this point, though, this is an ancient libvirt. I would suggest at least upgrading to the RHEL-5.4 version of libvirt, and see if you can reproduce the issue. If you can, then I would try again with the recently released 0.7.1, and if you are still having problems, then collect the information like I mentioned above. -- Chris Lalancette -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] 0.7.1 compile fails on opensuse 11.1
ACK to this patch btw, it does solve the problem in SuSE distros where device-mapper-devel is missing pkgconfig file. But the problem is really a bug in the devel package, so will this workaround be committed to libvirt configure script? Afaics, the opensuse patch has been sent and is now being reviewed. So hopefully this patch will not be needed (at least not for opensuse). Regards Dominik -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Miscellaneous fixes to build with -Werror
On Thu, Sep 24, 2009 at 01:02:24PM +0200, Chris Lalancette wrote:
Charles Duffy wrote:
HACKING suggests compiling with --enable-compile-warnings=error before
submitting any patches; however, current master fails for me on this
account (CentOS 5.3; gcc 4.1.2).
Please see attached. I suspect most of these should be uncontroversial
-- but wonder if perhaps virStrcpy uses would be better converted to
virStrcpyStatic rather than adding virStrcpy to the symbol list as done
here, and am curious about whether the need for explicit initialization
to silence a warning regarding qemudSetLogging's log_level indicates a
bug in the macro later used to assign that value.
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 2bae782..ec2eab1 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -2492,7 +2492,7 @@ remoteReadSaslAllowedUsernameList (virConfPtr conf
ATTRIBUTE_UNUSED,
*/
static int
qemudSetLogging(virConfPtr conf, const char *filename) {
-int log_level;
+int log_level = 0;
char *log_filters = NULL;
char *log_outputs = NULL;
int ret = -1;
Looking at this more, I'm not sure. The comment above GET_CONF_INT(log_level)
looks to be bogus; GET_CONF_INT does *not* return 0 if the value is not in the
config file, it doesn't change anything at all. Still, I don't quite know the
reasoning behind the original change (back in early August), so I'm
uncomfortable changing it.
Its pretty clear though that the 'log_level' var will never be initialized
if the 'log_level' setting isn't in the config file. ie,
GET_CONF_INTconf, filename, log_level);
expands to
virConfValuePtr p = virConfGetValue (conf, log_level);
if (p) {
if (checkType (p, filename, log_level, VIR_CONF_LONG) 0)
goto free_and_fail;
(log_level) = p-l;
}
So it is never set if 'p' is NULL
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 :|
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Remove hand-crafted UUID parsers
On Thu, Sep 24, 2009 at 10:13:15PM +0200, Matthias Bolte wrote:
2009/9/24 Daniel P. Berrange berra...@redhat.com:
* src/libvirt.c: Remove hand-crafted UUID parsers in favour of
calling virParseUUID
s/virParseUUID/virUUIDParse/
---
src/libvirt.c | 56
+---
1 files changed, 5 insertions(+), 51 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c
index 9fb0617..74d62a4 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -1839,26 +1839,11 @@ virDomainLookupByUUIDString(virConnectPtr conn,
const char *uuidstr)
virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
- /* XXX: sexpr_uuid() also supports '---' format.
- * We needn't it here. Right?
- */
- ret = sscanf(uuidstr,
- %02x%02x%02x%02x-
- %02x%02x-
- %02x%02x-
- %02x%02x-
- %02x%02x%02x%02x%02x%02x,
- raw + 0, raw + 1, raw + 2, raw + 3,
- raw + 4, raw + 5, raw + 6, raw + 7,
- raw + 8, raw + 9, raw + 10, raw + 11,
- raw + 12, raw + 13, raw + 14, raw + 15);
-
- if (ret!=VIR_UUID_BUFLEN) {
+
+ if (virUUIDParse(uuidstr, uuid) 0) {
virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
- for (i = 0; i VIR_UUID_BUFLEN; i++)
- uuid[i] = raw[i] 0xFF;
return virDomainLookupByUUID(conn, uuid[0]);
@@ -5038,26 +5023,10 @@ virNetworkLookupByUUIDString(virConnectPtr conn,
const char *uuidstr)
goto error;
}
- /* XXX: sexpr_uuid() also supports '---' format.
- * We needn't it here. Right?
- */
- ret = sscanf(uuidstr,
- %02x%02x%02x%02x-
- %02x%02x-
- %02x%02x-
- %02x%02x-
- %02x%02x%02x%02x%02x%02x,
- raw + 0, raw + 1, raw + 2, raw + 3,
- raw + 4, raw + 5, raw + 6, raw + 7,
- raw + 8, raw + 9, raw + 10, raw + 11,
- raw + 12, raw + 13, raw + 14, raw + 15);
-
- if (ret!=VIR_UUID_BUFLEN) {
+ if (virUUIDParse(uuidstr, uuid) 0) {
virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
- for (i = 0; i VIR_UUID_BUFLEN; i++)
- uuid[i] = raw[i] 0xFF;
return virNetworkLookupByUUID(conn, uuid[0]);
@@ -8887,26 +8856,11 @@ virSecretLookupByUUIDString(virConnectPtr conn,
const char *uuidstr)
virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
- /* XXX: sexpr_uuid() also supports '---' format.
- * We needn't it here. Right?
- */
- ret = sscanf(uuidstr,
- %02x%02x%02x%02x-
- %02x%02x-
- %02x%02x-
- %02x%02x-
- %02x%02x%02x%02x%02x%02x,
- raw + 0, raw + 1, raw + 2, raw + 3,
- raw + 4, raw + 5, raw + 6, raw + 7,
- raw + 8, raw + 9, raw + 10, raw + 11,
- raw + 12, raw + 13, raw + 14, raw + 15);
-
- if (ret!=VIR_UUID_BUFLEN) {
+
+ if (virUUIDParse(uuidstr, uuid) 0) {
virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
- for (i = 0; i VIR_UUID_BUFLEN; i++)
- uuid[i] = raw[i] 0xFF;
return virSecretLookupByUUID(conn, uuid[0]);
--
1.6.2.5
You removed the usage of raw, i and ret in this 3 functions, but
forgot to remove the variable definitions as well.
That's odd, the compiler didn't complain about those.
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 :|
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] node device devicekit backend
On Thu, Sep 24, 2009 at 02:08:00PM -0400, Dave Allan wrote: I've been looking at implementing a libudev backed node device, and I noticed that the devicekit node device backend doesn't build. Since I believe DeviceKit is deprecated for this sort of use, and AFAIK the DeviceKit support was never functionally equivalent to the HAL support, I'm wondering if we can just drop DeviceKit support. It also drags in glib, which is a separate problem. Opinions? Yes, we should kill the devicekit backend - upstream have pretty much killed the base devicekit daemon itself anyway telling people to use libudev instead. Devicekit was a still-born idea :-( 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/11] Helper functions for processing data streams in libvirtd
On Mon, Aug 24, 2009 at 09:51:06PM +0100, Daniel P. Berrange wrote: Defines the extensions to the remote protocol for generic data streams. Adds a bunch of helper code to the libvirtd daemon for working with data streams. * qemud/Makefile.am: Add stream.c/stream.h to build * qemud/stream.c, qemud/stream.h: Generic helper functions for creating new streams, associating streams with clients, finding existing streams for a client and removing/deleting streams. * qemud/remote_protocol.x: Add a new 'REMOTE_STREAM' constant for the 'enum remote_message_type' for encoding stream data in wire messages. Add a new 'REMOTE_CONTINUE' constant to 'enum remote_message_status' to indicate further data stream messsages are expected to follow. Document how the remote_message_header is used to encode data streams * qemud/remote_protocol.h: Regenerate * qemud/dispatch.c: Remove assumption that a error message sent to client is always type=REMOTE_REPLY. It may now also be type=REMOTE_STREAM. Add convenient method for sending outgoing stream data packets. Log and ignore non-filtered incoming stream packets. Add a method for serializing a stream error message * qemud/dispatch.h: Add API for serializing stream errors and sending stream data packets * qemud/qemud.h: Add struct qemud_client_stream for tracking active data streams for clients. Tweak filter function operation so that it accepts a client object too. * qemud/qemud.c: Refactor code for free'ing message objects which have been fully transmitted into separate method. Release all active streams when client shuts down. Change filter function to be responsible for queueing the message ACK, but this is hard to review, only testing can really bring confidence for this kind of code :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/11] Handle incoming data streams in libvirtd
On Mon, Aug 24, 2009 at 09:51:07PM +0100, Daniel P. Berrange wrote:
* qemud/stream.c: Handle incoming stream data packets, queuing until
stream becomes writable. Handle stream completion handshake
* po/POTFILES.in: Add qemud/stream.c
[...]
+/*
+ * Callback that gets invoked when a stream becomes writable/readable
+ */
+static void
+remoteStreamEvent(virStreamPtr st, int events, void *opaque)
+{
+struct qemud_client *client = opaque;
+struct qemud_client_stream *stream;
+
+/* XXX sub-optimal - we really should be taking the server lock
+ * first, but we have no handle to the server object
+ * We're lucky to get away with it for now, due to this callback
+ * executing in the main thread, but this should really be fixed
+ */
+virMutexLock(client-lock);
Shouldn't the stream stucture carry a reference to the
connection/server ?
[...]
static int
-remoteStreamFilter(struct qemud_client *client ATTRIBUTE_UNUSED,
- struct qemud_client_message *msg ATTRIBUTE_UNUSED,
- void *opaque ATTRIBUTE_UNUSED)
+remoteStreamFilter(struct qemud_client *client,
+ struct qemud_client_message *msg, void *opaque)
{
+struct qemud_client_stream *stream = opaque;
+
+if (msg-hdr.serial == stream-serial
+msg-hdr.proc == stream-procedure
+msg-hdr.type == REMOTE_STREAM) {
+DEBUG(Incoming rx=%p serial=%d proc=%d status=%d,
+ stream-rx, msg-hdr.proc, msg-hdr.serial, msg-hdr.status);
+
+/* If there are queued packets, we need to queue all further
+ * messages, since they must be processed strictly in order.
+ * If there are no queued packets, then OK/ERROR messages
+ * should be processed immediately. Data packets are still
+ * queued to only be processed when the stream is marked as
+ * writable.
+ */
Since we are already separating different classes of packets, why not
extend this to provide 2-3 classes for the data too: BACKGROUND/NORMAL/URGENT
that can be added later using the current flags for creating a stream.
ACK, though this need much testing, ideally we can automate this in
TCK, especially about Abort effect at various point in time of a
transfer. This is hard to test by the application writer so we should
try to make the API as foolproof as prossible.
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/11] Generic data stream handling
On Fri, Sep 25, 2009 at 09:58:51AM +0200, Daniel Veillard wrote: On Mon, Aug 24, 2009 at 09:51:03PM +0100, Daniel P. Berrange wrote: Okay, this is very similar in principle with HTTP pipelining with IMHO the same benefits and the same potential drawbacks. A couple of things to check might be: - the maximum amount of concurrent active streams allowed, for example suppose you want to migrate in emergency all the domains out of a failing machine, some level of serialization may be better than say attempting to migrate all 100 domains at the same time. 10 parallel stream might be better, but we need to make sure the API allows to report such condition. We could certainly add a tunable in /etc/libvirt/libvirtd.conf that limits the number of streams that are allowed per client. - the maximum chunking size, but with 256k I think this is covered. Yes, the remote protocol itself limits each message to 256k currently. I think this is a good enough size, since it avoids the stream delaying RPC calls, and the encryption chunk size is going to be smaller than this anyway, so you won't gain much from larger chunks. - synchronization internally between threads to avoid deadlocks or poor performances, that can be very hard to debug, so I guess an effort should be provided to explain how things are designed internally. Each individual virStreamPtr object is directly associated with a single API call, so in essence each virStreamPtr should only really be used from a single thread. That said, the virStreamPtr internal drivers should all lock the virStreamPtr object as they require to provide safety. 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/11] Add public API definition for data stream handling
On Fri, Sep 25, 2009 at 12:09:34PM +0200, Daniel Veillard wrote:
On Mon, Aug 24, 2009 at 09:51:04PM +0100, Daniel P. Berrange wrote:
@@ -1448,6 +1466,81 @@ void virEventRegisterImpl(virEventAddHandleFunc
addHandle,
virEventAddTimeoutFunc addTimeout,
virEventUpdateTimeoutFunc updateTimeout,
virEventRemoveTimeoutFunc removeTimeout);
+
+enum {
+VIR_STREAM_NONBLOCK = (1 0),
+};
+
+virStreamPtr virStreamNew(virConnectPtr conn,
+ unsigned int flags);
Would flags be sufficient if we were to encode some priorities to
streams? If we end up limiting the number of active streams, giving
higher priority or some reserved slots for quicker operations may
be important, flags should be sufficient for this if the need arise
so I don't see a problem but I raise the point.
I guess it would be sufficient if you wanted todo LOW/MEDIUM/HIGH
static priorties.
If we wanted to get more advanced, we could always introduce an
extra API, since there is always a point between virStreamNew()
and then using it, eg in virConnectUploadFile(), when you can
do more setup calls. We already allow event callbacks to be
registered this way, so we could in future add a
virStreamSetPriority(virStreamPtr st, options ...);
if we needed more than just plain flags.
On the subject of flags though, I've never been entirely sure
about whether it would be worth mandating the use of a flag(s)
for indicating I/O direction at time of stream creation.
Currently this is implicit base on the API that the stream is
later used with,
eg, currently
virStreamPtr st = virStreamNew(conn, 0);
virConnectUploadFile(conn, st, filename);
implicitly configures the stream for writing, but I constantly
wonder whether we ought to make it explicit via a flag like
virStreamPtr st = virStreamNew(conn, VIR_STREAM_WRITE);
virConnectUploadFile(conn, st, filename);
and require that the call of virStreamNew() provide either
VIR_STREAM_WRITE, or VIR_STREAM_READ, or both. ANd then also
have the methods using streams like virConnectUploadFile
check that the flags match.
If we wanted to mandate use of READ/WRITE flags for stream
creation, we'd obviously need todo it from teh start, since
we couldn't add that as a mandatory flag once the API is
released in use by apps.
+int virStreamRef(virStreamPtr st);
+
+int virStreamSend(virStreamPtr st,
+ const char *data,
+ size_t nbytes);
+
+int virStreamRecv(virStreamPtr st,
+ char *data,
+ size_t nbytes);
+
+
+typedef int (*virStreamSourceFunc)(virStreamPtr st,
+ char *data,
+ size_t nbytes,
+ void *opaque);
I think the signature is fine but we need to document all the
arguments and the return values.
+int virStreamSendAll(virStreamPtr st,
+ virStreamSourceFunc handler,
+ void *opaque);
Hum, I had to look at the comment to really understand. I'm not
100% sure, maybe we should allow for handler() to be called multiple
time, not just once.
For example I would allow virStreamSendAll() code to call handler
multiple time, until the handler like a read() returns 0 (or -1 on
error), in any case the signature should be documented fully.
This method is essentially just a convenient way to call the
virStreamSend() in a loop, for apps that are happy todo blocking
data I/O. As such the handler() will definitely be called multiple
times to fetch the data to be sent. You can see how it was used
later on in this patch, where virStreamSendAll is implemented.
I expect most apps would use virStreamSend() though, to allow
them todo interruptible non-blocking I/O
+typedef int (*virStreamSinkFunc)(virStreamPtr st,
+ const char *data,
+ size_t nbytes,
+ void *opaque);
Same thing do we allow a sink function to be called repeatedly ?
If we want to allow this in some ways we will need an extra argument
to indicate the end of the stream. Even if we don't plan this yet, I
would suggest to add a flags to allow for this possibility in the
future. With a chunk size of 256K at the protocol level it may not
be a good idea to keep the full data in memory, so I would allow
for this interface to call the sink multiple times. And IMHO it's best
to pass the indication of end of transfer directly at the sink level
rather than wait for the virStreamFree() coming from the user.
+int virStreamRecvAll(virStreamPtr st,
+ virStreamSinkFunc handler,
+ void *opaque);
Okay
Same as for SendAll, this API will invoke the handler multilpe
times to write out data that is being
[libvirt] [PATCH] Fix API doc extractor to stop munging comment formatting
The python method help docs are copied across from the C funtion comments, but in the process all line breaks and indentation was being lost. This made the resulting text and code examples completely unreadable. Both the API doc extractor and the python generator were destroying whitespace this fixes them to preserve it exactly. * docs/apibuild.py: Preserve all whitespace when extracting function comments. Print function comment inside a ![CDATA[ section to fully preserve all whitespace. Look for the word 'returns' to describe return values, instead of 'return' to avoid getting confused with code examples including the C 'return' statement. * python/generator.py: Preserve all whitespace when printing function help docs * src/libvirt.c: Change any return parameter indicated by 'return' to be 'returns', to avoid confusing the API extractor --- docs/apibuild.py| 31 +++- python/generator.py | 25 --- src/libvirt.c | 54 +- 3 files changed, 61 insertions(+), 49 deletions(-) diff --git a/docs/apibuild.py b/docs/apibuild.py index 855569b..70a7efc 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -838,14 +838,20 @@ class CParser: arg, name)) while len(lines) 0 and lines[0] == '*': del lines[0] - desc = + desc = None while len(lines) 0: l = lines[0] - while len(l) 0 and l[0] == '*': - l = l[1:] - l = string.strip(l) - if len(l) = 6 and l[0:6] == return or l[0:6] == Return: - try: + i = 0 + # Remove all leading '*', followed by at most one ' ' character + # since we need to preserve correct identation of code examples + while i len(l) and l[i] == '*': + i = i + 1 + if i 0: + if i len(l) and l[i] == ' ': + i = i + 1 + l = l[i:] + if len(l) = 6 and l[0:7] == returns or l[0:7] == Returns: + try: l = string.split(l, ' ', 1)[1] except: l = @@ -859,9 +865,14 @@ class CParser: retdesc = retdesc + + l del lines[0] else: - desc = desc + + l + if desc is not None: + desc = desc + \n + l + else: + desc = l del lines[0] + if desc is None: + desc = retdesc = string.strip(retdesc) desc = string.strip(desc) @@ -1716,7 +1727,7 @@ class docBuilder: try: (args, desc) = id.info if desc != None and desc != : - output.write( info%s/info\n % (escape(desc))) + output.write( info![CDATA[%s]]/info\n % (desc)) self.indexString(name, desc) for arg in args: (name, desc) = arg @@ -1760,7 +1771,7 @@ class docBuilder: try: desc = id.extra if desc != None and desc != : - output.write(\n info%s/info\n % (escape(desc))) + output.write(\n info![CDATA[%s]]/info\n % (desc)) output.write(/typedef\n) else: output.write(/\n) @@ -1796,7 +1807,7 @@ class docBuilder: output.write( cond%s/cond\n% (apstr)); try: (ret, params, desc) = id.info - output.write( info%s/info\n % (escape(desc))) + output.write( info![CDATA[%s]]/info\n % (desc)) self.indexString(name, desc) if ret[0] != None: if ret[0] == void: diff --git a/python/generator.py b/python/generator.py index c34cb34..178a415 100755 --- a/python/generator.py +++ b/python/generator.py @@ -44,6 +44,7 @@ if sgmlop: self.finish_starttag = target.start self.finish_endtag = target.end self.handle_data = target.data +self.handle_cdata = target.cdata # activate parser self.parser = sgmlop.XMLParser() @@ -78,6 +79,7 @@ class SlowParser(xmllib.XMLParser): def __init__(self, target): self.unknown_starttag = target.start self.handle_data = target.data +self.handle_cdata = target.cdata self.unknown_endtag = target.end xmllib.XMLParser.__init__(self) @@ -108,6 +110,11 @@ class docParser: print data %s % text self._data.append(text) +def cdata(self, text): +if debug: +print data %s % text +self._data.append(text) + def start(self, tag, attrs): if debug: print start %s, %s % (tag, attrs) @@ -843,20 +850,14 @@ def writeDoc(name, args, indent, output): val =
Re: [libvirt] [PATCH 01/11] Add public API definition for data stream handling
On Fri, Sep 25, 2009 at 12:25:50PM +0100, Daniel P. Berrange wrote:
On Fri, Sep 25, 2009 at 12:09:34PM +0200, Daniel Veillard wrote:
On Mon, Aug 24, 2009 at 09:51:04PM +0100, Daniel P. Berrange wrote:
@@ -1448,6 +1466,81 @@ void virEventRegisterImpl(virEventAddHandleFunc
addHandle,
virEventAddTimeoutFunc addTimeout,
virEventUpdateTimeoutFunc updateTimeout,
virEventRemoveTimeoutFunc removeTimeout);
+
+enum {
+VIR_STREAM_NONBLOCK = (1 0),
+};
+
+virStreamPtr virStreamNew(virConnectPtr conn,
+ unsigned int flags);
Would flags be sufficient if we were to encode some priorities to
streams? If we end up limiting the number of active streams, giving
higher priority or some reserved slots for quicker operations may
be important, flags should be sufficient for this if the need arise
so I don't see a problem but I raise the point.
I guess it would be sufficient if you wanted todo LOW/MEDIUM/HIGH
static priorties.
yeah, I just wanted to make sure it didn't contradict something you
may have in mind :-)
If we wanted to get more advanced, we could always introduce an
extra API, since there is always a point between virStreamNew()
and then using it, eg in virConnectUploadFile(), when you can
do more setup calls. We already allow event callbacks to be
registered this way, so we could in future add a
virStreamSetPriority(virStreamPtr st, options ...);
right,
if we needed more than just plain flags.
On the subject of flags though, I've never been entirely sure
about whether it would be worth mandating the use of a flag(s)
for indicating I/O direction at time of stream creation.
Currently this is implicit base on the API that the stream is
later used with,
eg, currently
virStreamPtr st = virStreamNew(conn, 0);
virConnectUploadFile(conn, st, filename);
implicitly configures the stream for writing, but I constantly
wonder whether we ought to make it explicit via a flag like
virStreamPtr st = virStreamNew(conn, VIR_STREAM_WRITE);
virConnectUploadFile(conn, st, filename);
and require that the call of virStreamNew() provide either
VIR_STREAM_WRITE, or VIR_STREAM_READ, or both. ANd then also
have the methods using streams like virConnectUploadFile
check that the flags match.
Hum, this would then raise the signal that stream can be used
both ways, do we really want to suggest this at the API level,
I can see how we're gonna use this internally, but aren't we opening
the door to much complexity ?
If we wanted to mandate use of READ/WRITE flags for stream
creation, we'd obviously need todo it from teh start, since
we couldn't add that as a mandatory flag once the API is
released in use by apps.
yes that's a good point, a design issue too. If you really expect API
usage for both read and write, then I would say we should make those
flags mandatory. The only point is that our existing flags use in APIs
are just fine with 0 as being the 'default', and we would break this
but it's not a big deal IMHO, that will be caught immediately.
+int virStreamRef(virStreamPtr st);
+
+int virStreamSend(virStreamPtr st,
+ const char *data,
+ size_t nbytes);
+
+int virStreamRecv(virStreamPtr st,
+ char *data,
+ size_t nbytes);
+
+
+typedef int (*virStreamSourceFunc)(virStreamPtr st,
+ char *data,
+ size_t nbytes,
+ void *opaque);
I think the signature is fine but we need to document all the
arguments and the return values.
+int virStreamSendAll(virStreamPtr st,
+ virStreamSourceFunc handler,
+ void *opaque);
Hum, I had to look at the comment to really understand. I'm not
100% sure, maybe we should allow for handler() to be called multiple
time, not just once.
For example I would allow virStreamSendAll() code to call handler
multiple time, until the handler like a read() returns 0 (or -1 on
error), in any case the signature should be documented fully.
This method is essentially just a convenient way to call the
virStreamSend() in a loop, for apps that are happy todo blocking
data I/O. As such the handler() will definitely be called multiple
times to fetch the data to be sent. You can see how it was used
later on in this patch, where virStreamSendAll is implemented.
I expect most apps would use virStreamSend() though, to allow
them todo interruptible non-blocking I/O
Okay, but let's describe all args and return values for the callbacks.
+typedef int (*virStreamSinkFunc)(virStreamPtr st,
+ const char *data,
+ size_t
Re: [libvirt] [PATCH] Remove hand-crafted UUID parsers
On Fri, Sep 25, 2009 at 10:41:08AM +0100, Daniel P. Berrange wrote:
On Thu, Sep 24, 2009 at 10:13:15PM +0200, Matthias Bolte wrote:
2009/9/24 Daniel P. Berrange berra...@redhat.com:
* src/libvirt.c: Remove hand-crafted UUID parsers in favour of
calling virParseUUID
s/virParseUUID/virUUIDParse/
---
src/libvirt.c | 56
+---
1 files changed, 5 insertions(+), 51 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c
index 9fb0617..74d62a4 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -1839,26 +1839,11 @@ virDomainLookupByUUIDString(virConnectPtr conn,
const char *uuidstr)
virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
- /* XXX: sexpr_uuid() also supports '---' format.
- * We needn't it here. Right?
- */
- ret = sscanf(uuidstr,
- %02x%02x%02x%02x-
- %02x%02x-
- %02x%02x-
- %02x%02x-
- %02x%02x%02x%02x%02x%02x,
- raw + 0, raw + 1, raw + 2, raw + 3,
- raw + 4, raw + 5, raw + 6, raw + 7,
- raw + 8, raw + 9, raw + 10, raw + 11,
- raw + 12, raw + 13, raw + 14, raw + 15);
-
- if (ret!=VIR_UUID_BUFLEN) {
+
+ if (virUUIDParse(uuidstr, uuid) 0) {
virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
- for (i = 0; i VIR_UUID_BUFLEN; i++)
- uuid[i] = raw[i] 0xFF;
return virDomainLookupByUUID(conn, uuid[0]);
@@ -5038,26 +5023,10 @@ virNetworkLookupByUUIDString(virConnectPtr conn,
const char *uuidstr)
goto error;
}
- /* XXX: sexpr_uuid() also supports '---' format.
- * We needn't it here. Right?
- */
- ret = sscanf(uuidstr,
- %02x%02x%02x%02x-
- %02x%02x-
- %02x%02x-
- %02x%02x-
- %02x%02x%02x%02x%02x%02x,
- raw + 0, raw + 1, raw + 2, raw + 3,
- raw + 4, raw + 5, raw + 6, raw + 7,
- raw + 8, raw + 9, raw + 10, raw + 11,
- raw + 12, raw + 13, raw + 14, raw + 15);
-
- if (ret!=VIR_UUID_BUFLEN) {
+ if (virUUIDParse(uuidstr, uuid) 0) {
virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
- for (i = 0; i VIR_UUID_BUFLEN; i++)
- uuid[i] = raw[i] 0xFF;
return virNetworkLookupByUUID(conn, uuid[0]);
@@ -8887,26 +8856,11 @@ virSecretLookupByUUIDString(virConnectPtr conn,
const char *uuidstr)
virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
- /* XXX: sexpr_uuid() also supports '---' format.
- * We needn't it here. Right?
- */
- ret = sscanf(uuidstr,
- %02x%02x%02x%02x-
- %02x%02x-
- %02x%02x-
- %02x%02x-
- %02x%02x%02x%02x%02x%02x,
- raw + 0, raw + 1, raw + 2, raw + 3,
- raw + 4, raw + 5, raw + 6, raw + 7,
- raw + 8, raw + 9, raw + 10, raw + 11,
- raw + 12, raw + 13, raw + 14, raw + 15);
-
- if (ret!=VIR_UUID_BUFLEN) {
+
+ if (virUUIDParse(uuidstr, uuid) 0) {
virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
- for (i = 0; i VIR_UUID_BUFLEN; i++)
- uuid[i] = raw[i] 0xFF;
return virSecretLookupByUUID(conn, uuid[0]);
--
1.6.2.5
You removed the usage of raw, i and ret in this 3 functions, but
forgot to remove the variable definitions as well.
That's odd, the compiler didn't complain about those.
in any case, ACK !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Re: [PATCH] Fix API doc extractor to stop munging comment formatting
On Fri, Sep 25, 2009 at 01:28:51PM +0100, Daniel P. Berrange wrote: The python method help docs are copied across from the C funtion comments, but in the process all line breaks and indentation was being lost. This made the resulting text and code examples completely unreadable. Both the API doc extractor and the python generator were destroying whitespace this fixes them to preserve it exactly. I meant to include an example of the difference when I sent this. eg, launch python, type 'import libvirt' and then 'help(libvirt)'. For the 'migrate' API currently you see | migrate(self, domain, flags, dname, uri, bandwidth) | Migrate the domain object from its current host to the | destination host given by dconn (a connection to the | destination host). Flags may be one of more of the | following: VIR_MIGRATE_LIVE Attempt a live migration. If | a hypervisor supports renaming domains during migration, | then you may set the dname parameter to the new name | (otherwise it keeps the same name). If this is not | supported by the hypervisor, dname must be None or else you | will get an error. Since typically the two hypervisors | connect directly to each other in order to perform the | migration, you may need to specify a path from the source | to the destination. This is the purpose of the uri | parameter. If uri is None, then libvirt will try to find | the best method. Uri may specify the hostname or IP | address of the destination host as seen from the source. | Or uri may be a URI giving transport, hostname, user, port, | etc. in the usual form. Refer to driver documentation for | the particular URIs supported. The maximum bandwidth (in | Mbps) that will be used to do migration can be specified | with the bandwidth parameter. If set to 0, libvirt will | choose a suitable default. Some hypervisors do not support | this feature and will return an error if bandwidth is not | 0. To see which features are supported by the current | hypervisor, see virConnectGetCapabilities, | /capabilities/host/migration_features. There are many | limitations on migration imposed by the underlying | technology - for example it may not be possible to migrate | between different processors even with the same | architecture, or between different types of hypervisor. After my patch you get | migrate(self, domain, flags, dname, uri, bandwidth) | Migrate the domain object from its current host to the destination | host given by dconn (a connection to the destination host). | | Flags may be one of more of the following: |VIR_MIGRATE_LIVE Attempt a live migration. | | If a hypervisor supports renaming domains during migration, | then you may set the dname parameter to the new name (otherwise | it keeps the same name). If this is not supported by the | hypervisor, dname must be None or else you will get an error. | | Since typically the two hypervisors connect directly to each | other in order to perform the migration, you may need to specify | a path from the source to the destination. This is the purpose | of the uri parameter. If uri is None, then libvirt will try to | find the best method. Uri may specify the hostname or IP address | of the destination host as seen from the source. Or uri may be | a URI giving transport, hostname, user, port, etc. in the usual | form. Refer to driver documentation for the particular URIs | supported. | | The maximum bandwidth (in Mbps) that will be used to do migration | can be specified with the bandwidth parameter. If set to 0, | libvirt will choose a suitable default. Some hypervisors do | not support this feature and will return an error if bandwidth | is not 0. | | To see which features are supported by the current hypervisor, | see virConnectGetCapabilities, /capabilities/host/migration_features. | | There are many limitations on migration imposed by the underlying | technology - for example it may not be possible to migrate between | different processors even with the same architecture, or between | different types of hypervisor. 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
Re: [libvirt] [PATCH 5/9] Implement controller hotplugging
Daniel P. Berrange wrote:
On Fri, Sep 18, 2009 at 05:26:12PM +0200, Wolfgang Mauerer wrote:
This enables to hot-add disk controllers without attached
disks into the system. Previously, it was only possible to
(implicitly) add disk controllers in the static machine
configuration.
Notice that the actual functionality is only available
for qemu at present, but other emulators can be extended
likewise.
Any idea what we can do about initial startup? eg, if we start
a guest with 1 SCSI controller and 1 disk, and then we've then
hotplugged a 2nd controller, and 1 disk. When we later boot
or migrate the same guest, we need to have suitable QEMU command
line args to make sure we get 2 controllers each with the same
disk, rather than 1 controller with 2 disks. I'm not sure how
we do this in QEMU, hopefully the existing args support it in
some way I've not realized, or failing that the new qdev -device
args might help us.
I'm not yet sure how to solve that best. I'll take a look at how
the -device can help exactly.
Signed-off-by: Wolfgang Mauerer wolfgang.maue...@siemens.com
Signed-off-by: Jan Kiszka jan.kis...@siemens.com
---
src/domain_conf.c| 26 +++---
src/domain_conf.h|2 ++
src/libvirt_private.syms |1 +
src/qemu_driver.c| 21 +
4 files changed, 43 insertions(+), 7 deletions(-)
diff --git a/src/domain_conf.c b/src/domain_conf.c
index d0fda64..ea51fda 100644
--- a/src/domain_conf.c
+++ b/src/domain_conf.c
@@ -647,7 +647,6 @@ void virDomainRemoveInactive(virDomainObjListPtr doms,
}
-
/* Parse the XML definition for a disk
* @param node XML nodeset to parse for disk definition
*/
@@ -2554,6 +2553,27 @@ virDomainDeviceDefPtr
virDomainDeviceDefParse(virConnectPtr conn,
#endif
+static int virDomainControllerCompare(virDomainControllerDefPtr a,
+ virDomainControllerDefPtr b) {
+if (a-pci_addr.bus == b-pci_addr.bus) {
+if (a-pci_addr.domain == b-pci_addr.domain)
+return a-pci_addr.slot - b-pci_addr.slot;
+
+return a-pci_addr.domain - b-pci_addr.domain;
+}
+
+return a-pci_addr.bus - b-pci_addr.bus;
+}
+
+
+int virDomainControllerQSort(const void *a, const void *b)
+{
+const virDomainControllerDefPtr *da = a;
+const virDomainControllerDefPtr *db = b;
+
+return virDomainControllerCompare(*da, *db);
+}
+
I know we used todo this for disk devices, but I'd recommand not
going a qsort of devices when hotplugging/unplugging. For hotplug
always append to the list, for unplug just shuffle down later
devices in the list to fill the hole.
okay.
Thanks, Wolfgang
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 9/9] Implement disk- and controller hotremove
Daniel P. Berrange wrote:
On Fri, Sep 18, 2009 at 05:26:16PM +0200, Wolfgang Mauerer wrote:
Since both disks and disk controllers can be dynamically
added to the system, it makes sense to be also able to remove
them.
This is the main patch which requires additional support in QEMu
if I'm understanding your patcheset to qemu-devel correctly.
Yes, that's right.
diff --git a/src/domain_conf.h b/src/domain_conf.h
index 17f8b14..c7d49cf 100644
--- a/src/domain_conf.h
+++ b/src/domain_conf.h
@@ -149,6 +149,14 @@ virDiskHasValidPciAddr(virDomainDiskDefPtr def)
return def-pci_addr.domain || def-pci_addr.domain ||
def-pci_addr.slot;
}
+static inline int
+virDiskHasValidController(virDomainDiskDefPtr def)
+{
+return def-controller_id != NULL ||
+def-controller_pci_addr.domain || def-controller_pci_addr.domain
+|| def-controller_pci_addr.slot;
+}
+
/* Two types of disk backends */
enum virDomainFSType {
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index ddc46f6..5ac89a1 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -6204,32 +6204,31 @@ cleanup:
return ret;
}
-static int qemudDomainDetachPciDiskDevice(virConnectPtr conn,
- virDomainObjPtr vm,
virDomainDeviceDefPtr dev)
+static int qemudDomainDetachDiskController(virConnectPtr conn,
+ virDomainObjPtr vm,
virDomainDeviceDefPtr dev)
{
int i, ret = -1;
char *cmd = NULL;
char *reply = NULL;
-virDomainDiskDefPtr detach = NULL;
+virDomainControllerDefPtr detach = NULL;
int tryOldSyntax = 0;
-for (i = 0 ; i vm-def-ndisks ; i++) {
-if (STREQ(vm-def-disks[i]-dst, dev-data.disk-dst)) {
-detach = vm-def-disks[i];
+//virDomainControllerDefPtr is dev-data.controller
+for (i = 0 ; i vm-def-ncontrollers ; i++) {
+if (vm-def-controllers[i]-pci_addr.domain ==
+dev-data.controller-pci_addr.domain
+vm-def-controllers[i]-pci_addr.bus ==
+dev-data.controller-pci_addr.bus
+vm-def-controllers[i]-pci_addr.slot ==
+dev-data.controller-pci_addr.slot) {
+detach = vm-def-controllers[i];
break;
}
}
if (!detach) {
qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
- _(disk %s not found), dev-data.disk-dst);
-goto cleanup;
-}
-
-if (!virDiskHasValidPciAddr(detach)) {
-qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
- _(disk %s cannot be detached - no PCI address for
device),
- detach-dst);
+ _(Controller %s not found), dev-data.disk-dst);
goto cleanup;
}
@@ -6251,7 +6250,9 @@ try_command:
if (qemudMonitorCommand(vm, cmd, reply) 0) {
qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
- _(failed to execute detach disk %s command),
detach-dst);
+ _(failed to execute detach controller %d:%d:%d \
+ command), detach-pci_addr.domain,
+ detach-pci_addr.bus, detach-pci_addr.slot);
goto cleanup;
}
@@ -6267,6 +6268,124 @@ try_command:
if (strstr(reply, invalid slot) ||
strstr(reply, Invalid pci address)) {
qemudReportError (conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
+ _(failed to detach controller: invalid PCI
address %.4x:%.2x:%.2x: %s),
+ detach-pci_addr.domain,
+ detach-pci_addr.bus,
+ detach-pci_addr.slot,
+ reply);
+goto cleanup;
+}
+
+if (vm-def-ncontrollers 1) {
+vm-def-controllers[i] =
vm-def-controllers[--vm-def-ncontrollers];
+if (VIR_REALLOC_N(vm-def-controllers, vm-def-ncontrollers) 0)
{
+virReportOOMError(conn);
+goto cleanup;
+}
+qsort(vm-def-disks, vm-def-ncontrollers,
+ sizeof(*vm-def-controllers),
+ virDomainControllerQSort);
+} else {
+VIR_FREE(vm-def-controllers[0]);
+vm-def-controllers = 0;
+}
+ret = 0;
+
+cleanup:
+VIR_FREE(reply);
+VIR_FREE(cmd);
+return ret;
+}
+
+static int qemudDomainDetachDiskDevice(virConnectPtr conn,
+ virDomainObjPtr vm,
virDomainDeviceDefPtr dev)
+{
+int i, ret = -1;
+char *cmd = NULL;
+char *reply = NULL;
+const char* type;
+virDomainDiskDefPtr detach = NULL;
+int tryOldSyntax = 0;
+
+/* If bus and unit are specified, use these first to identify
+ the disk */
+if (dev-data.disk-controller_pci_addr.domain != -1) {
+
Re: [libvirt] [PATCH 3/9] Add new domain device: controller
Daniel P. Berrange wrote:
On Fri, Sep 18, 2009 at 05:26:10PM +0200, Wolfgang Mauerer wrote:
This augments virDomainDevice with a controller element
that is used to represent disk controllers (e.g., scsi
controllers). The XML format is given by
controller type=scsi id=my_id
bus addr=Domain:Bus:Slot
/controller
where type denotes the disk interface (scsi, ide,...), id
is an arbitrary string that identifies the controller for
disk hotadd/remove, and bus addr denotes the controller address
on the PCI bus.
The bus element can be omitted; in this case,
an address will be assigned automatically. Currently,
only hotplugging scsi controllers on a PCI bus
is supported, and this only for qemu guests
As mentioned in the previous patch, I reckon 'id' is better
called 'name'.
For PCI addresses, it is desirable to fully normalize the XML
format, by which I mean have separate attributes for domain,
bus and slot. We already have a syntax for PCI addresses used
for host device passthrough, so it'd make sense to use the
same syntax here for controllers. More broadly, we're probably
going to have to add a PCI address element to all our devices.
While it is unlikely we'll need non-PCI addresses, it doesn't
hurt to make it explicit by adding a type='pci' attribute
Thus I'd suggest
address type='pci' domain='0x' bus='0x06' slot='0x12'/
Instead of
bus addr=Domain:Bus:Slot
In the domain_conf.c/.h parser, we could have a datatype like
enum {
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI,
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST
};
typedef struct _virDomainDevicePCIAddress virDomainDevicePCIAddress;
struct _virDomainDevicePCIAddress {
unsigned domain;
unsigned bus;
unsigned slot;
unsigned function;
};
typedef struct _virDomainDeviceAddress virDomainDeviceAddress;
struct _virDomainDeviceAddress {
int type;
union {
virDomainDevicePCIAddress pci;
} data;
};
Which we then use in all the places in domain_conf.h wanting
address information. Obviously we'd not use the 'function'
field in most places, but doesn't hurt to have it.
And a pair of methods for parsing/formatting this address info
we can call from all the appropriate locations.
using a generic format for PCi addresses surely makes sense,
especially wrt. the common data structure and parsing routines -
I'll add these into the next series. However, I'm not sure
if using address type='pci' domain='0x' bus='0x06' slot='0x12'/
instead of a simple string provides much of an advantage
and does not just increase the typing strain (though there are
maybe some small advantages for computerised parsing of libvirt
configuration files by other apps).
There's already a generic definition of PCI addresses in
docs/schemas/domain.rng:\approx 1140, I suppose it's your
intention to extend if by a type field?
Thanks, Wolfgang
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/9] Extend disk element with controller information
Matthias Bolte wrote: 2009/9/24 Daniel P. Berrange berra...@redhat.com: Outside the scope of your patches, I think it would be worth adding a 'name' attribute to all devices in the libvirt XML as a standardized unique identifier. We already have to keep track of a 'name' internally for NIC hotplug with QEMU, and with QEMU's qdev work we're going to end up having to track a 'name' for pretty much all devices. Thus we might as well expose it in the XML Would this device name be read-only, or would it be editable/writable by the user (e.g. through virsh edit)? If it should be read-only, I see no problem supporting device names in the ESX driver. But If it should be editable/writable it may be a problem to implement this for the ESX driver, as I'm currently not aware of any sensible way to store such an custom device name in a persistent manner on an ESX server. I suppose having the name read-only suffices. At least I don't see any particularly important use cases for a writeable name, especially considering that the name can be chosen arbitrarily at creation time. And there's still the possibility to shut down the virtual machine, change names, and restart again if new names are absolutely required. Thanks, Wolfgang -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/11] Add public API definition for data stream handling
On Fri, Sep 25, 2009 at 02:32:39PM +0200, Daniel Veillard wrote: On Fri, Sep 25, 2009 at 12:25:50PM +0100, Daniel P. Berrange wrote: if we needed more than just plain flags. On the subject of flags though, I've never been entirely sure about whether it would be worth mandating the use of a flag(s) for indicating I/O direction at time of stream creation. Currently this is implicit base on the API that the stream is later used with, eg, currently virStreamPtr st = virStreamNew(conn, 0); virConnectUploadFile(conn, st, filename); implicitly configures the stream for writing, but I constantly wonder whether we ought to make it explicit via a flag like virStreamPtr st = virStreamNew(conn, VIR_STREAM_WRITE); virConnectUploadFile(conn, st, filename); and require that the call of virStreamNew() provide either VIR_STREAM_WRITE, or VIR_STREAM_READ, or both. ANd then also have the methods using streams like virConnectUploadFile check that the flags match. Hum, this would then raise the signal that stream can be used both ways, do we really want to suggest this at the API level, I can see how we're gonna use this internally, but aren't we opening the door to much complexity ? Yeah, that's more or less why I left it out so far - I've not yet found a case where I absolutely needed the WRITE/READ flags to be set explicitly by apps. If we wanted to mandate use of READ/WRITE flags for stream creation, we'd obviously need todo it from teh start, since we couldn't add that as a mandatory flag once the API is released in use by apps. yes that's a good point, a design issue too. If you really expect API usage for both read and write, then I would say we should make those flags mandatory. The only point is that our existing flags use in APIs are just fine with 0 as being the 'default', and we would break this but it's not a big deal IMHO, that will be caught immediately. There is one likely API where we'd have a full read+write stream. I've thought about adding ability to tunnel a serial port PTY over libvirt, so 'virsh console' could be made to work remotely. eg, something like virDomainOpenConsole(virDomainPtr dom, virStreamPtr stream const char *consolename); In this case you'd be reading writing from /to the same stream. It still wouldn't really require that we set the READ+WRITE flags when doing virStreamNew() +typedef int (*virStreamSinkFunc)(virStreamPtr st, + const char *data, + size_t nbytes, + void *opaque); Same thing do we allow a sink function to be called repeatedly ? If we want to allow this in some ways we will need an extra argument to indicate the end of the stream. Even if we don't plan this yet, I would suggest to add a flags to allow for this possibility in the future. With a chunk size of 256K at the protocol level it may not be a good idea to keep the full data in memory, so I would allow for this interface to call the sink multiple times. And IMHO it's best to pass the indication of end of transfer directly at the sink level rather than wait for the virStreamFree() coming from the user. +int virStreamRecvAll(virStreamPtr st, + virStreamSinkFunc handler, + void *opaque); Okay Same as for SendAll, this API will invoke the handler multilpe times to write out data that is being received. In both cases the implementation is invoking the handler with 64kb buffers to avoid pulling lots of data into memory. Okay, but I think being able to indicate there that a packet is the last one may be important, for example if the application design prefer to initiate the closure of the transfer (close/sync/...) as soon as possible. Actually in the case of the 'source' function, the app already knows when its got to the end, because its source funtion will be returning '0' for end-of-file. For the 'sink' function we'd have to make sure we called it once at the end with a length of '0' to indicate EOF in that direction. I can't remember offhand if we'll do that already or not. Also how flexible are we in the design with callbacks taling a long time to complete, for example reads crossing the network, or slow output devices ? Maybe this should be hinted in the callback descriptions. These callbacks are the app's responsibility execute in its context, so libvirt doesn't care whether they are slow or fast to execute. Everything internal to libvirt relating to streams is non-blocking fast. + * virConnectUploadFile(conn, st); + * virStreamSendAll(st, mysource, fd); + * virStreamFree(st); + * close(fd); + * virConnectUploadFile(conn, st); + * virStreamRecvAll(st, mysink, fd); + * virStreamFree(st);
Re: [libvirt] [PATCH 2/9] Extend disk element with controller information
On Thu, Sep 24, 2009 at 11:39:08PM +0200, Matthias Bolte wrote: 2009/9/24 Daniel P. Berrange berra...@redhat.com: Outside the scope of your patches, I think it would be worth adding a 'name' attribute to all devices in the libvirt XML as a standardized unique identifier. We already have to keep track of a 'name' internally for NIC hotplug with QEMU, and with QEMU's qdev work we're going to end up having to track a 'name' for pretty much all devices. Thus we might as well expose it in the XML Would this device name be read-only, or would it be editable/writable by the user (e.g. through virsh edit)? If it should be read-only, I see no problem supporting device names in the ESX driver. But If it should be editable/writable it may be a problem to implement this for the ESX driver, as I'm currently not aware of any sensible way to store such an custom device name in a persistent manner on an ESX server. Latest QEMU allows the name to be choosen by the application arbitrarily, but older QEMU auto-assigned names itself. I don't see any compelling reason to make the names editable by applications, so it is probably sufficient to say they'll be read-only / output-only attributes in the XML assigned by the hypervisor driver as required. 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Support a new peer-to-peer migration mode public API
On Tue, Sep 22, 2009 at 02:43:20PM +0200, Daniel Veillard wrote:
[ Sending again due to mail output problems ]
On Thu, Sep 17, 2009 at 06:25:01PM +0100, Daniel P. Berrange wrote:
Introduces several new public API options for migration
- VIR_MIGRATE_PEER2PEER: With this flag the client only
invokes the virDomainMigratePerform method, expecting
the source host driver to do whatever is required to
complete the entire migration process.
- VIR_MIGRATE_TUNNELLED: With this flag the actual data
for migration will be tunnelled over the libvirtd RPC
channel. This requires that VIR_MIGRATE_PEER2PEER is
also set.
Hum, I would rather add that when entering libvirt.c entry point
set explicitely VIR_MIGRATE_PEER2PEER if VIR_MIGRATE_TUNNELLED is asked
for, that's an internal impl. detail.
I didn't do that, because I wanted to leave open the option
to supporting TUNNELLED mode, without PEER2PEER mode. I
don't really know what that would look like, but I think it
is better future proofing to not presume one implies the other.
- virDomainMigrateToURI: This is variant of the existing
virDomainMigrate method which can be used when the
VIR_MIGRATE_PEER2PEER flag is set. The benefit of this
method is that no virConnectPtr object is required for
the destination host, only a destination URI.
Sounds good if we have proper error handling. I'm also wondering
how authentification and access control can be handled in that mode,
the normal method of access somehow garantee that the user credentials
can be checked in some ways, but if it's direct daemon to daemon
talking how can this be performed ?
The client application authenticates with the source libvirtd
in the normal manner. The source libvirtd will authenticate
with the destination libvirtd directly.
The client application will not be authenticating with the
destination libvirtd itself.
While this may sound like a security issue, I don't think it
really is. It is more of an authorization problem - eg the
admin needs to declare somewhere which libvirtd instances
are allowed to migrate guests between then. This authorization
problem is really outside the scope of the client application
or our public app API
+int virDomainMigrateToURI (virDomainPtr domain, const char *duri,
+ unsigned long flags, const char *dname,
+ unsigned long bandwidth);
+
s/duri/dest/ or target, the fact it's a connection URI should be made
obvious in the function comment.
Yep, the function comment later only clearly describes what URIs are
acceptable.
[...]
+} else {
+if (flags VIR_MIGRATE_TUNNELLED) {
+virLibConnError(domain-conn, VIR_ERR_OPERATION_INVALID,
+_(cannot perform tunnelled migration without
using peer2peer flag));
+goto error;
+}
I don't really agree, we should just or the flag and hide this from
the API user.
I really did want to leave open the option of relaxing this restriction
in the future, and thus didn't want to automatically set the PEER2PEER
flag - once its set we can never unset it in the future.
+
+
/*
* Not for public use. This function is part of the internal
* implementation of migration in the remote case.
diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h
index 5913798..5f1a7fe 100644
--- a/src/libvirt_internal.h
+++ b/src/libvirt_internal.h
@@ -17,6 +17,7 @@
* License along with this library; if not, write to the Free Software
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*
+ * NB This file is (secret) ABI sensitive. Append only
Huh ???
The methods here are encoded in the wire protocol ABI,
but not included in the public libvirt.h header. Hence
they are ABI sensitive, but secret thus append only
like the rest of our API
diff --git a/src/xen_unified.c b/src/xen_unified.c
index dfa9ca5..954b187 100644
--- a/src/xen_unified.c
+++ b/src/xen_unified.c
@@ -455,8 +455,11 @@ static int
xenUnifiedSupportsFeature (virConnectPtr conn ATTRIBUTE_UNUSED, int
feature)
{
switch (feature) {
-case VIR_DRV_FEATURE_MIGRATION_V1: return 1;
-default: return 0;
+case VIR_DRV_FEATURE_MIGRATION_V1:
+case VIR_DRV_FEATURE_MIGRATION_P2P:
+return 1;
+default:
+return 0;
}
}
diff --git a/src/xend_internal.c b/src/xend_internal.c
index 7f55116..da5c039 100644
--- a/src/xend_internal.c
+++ b/src/xend_internal.c
@@ -4400,6 +4400,12 @@ xenDaemonDomainMigratePerform (virDomainPtr domain,
strcpy (live, 1);
flags = ~VIR_MIGRATE_LIVE;
}
+/* Trivially support this in Xen, since XenD on dest is always
+ * ready to accept incoming migration */
+if ((flags VIR_MIGRATE_PEER2PEER)) {
+flags = ~VIR_MIGRATE_PEER2PEER;
[libvirt] [PATCH 3/7] Move file format enum to libvirt_util
Renam virStorageVolFormatFileSystem to virStorageFileFormat and
move to src/util/storage_file.[ch]
* src/Makefile.am: add src/util/storage_file.[ch]
* src/conf/storage_conf.[ch]: move enum from here ...
* src/util/storage_file.[ch]: .. to here
* src/libvirt_private.syms: update To/FromString exports
* src/storage/storage_backend.c, src/storage/storage_backend_fs.c,
src/vbox/vbox_tmpl.c: update for above changes
---
src/Makefile.am |1 +
src/conf/storage_conf.c | 25
src/conf/storage_conf.h | 17 --
src/libvirt_private.syms |5 ++-
src/storage/storage_backend.c| 17 +++--
src/storage/storage_backend_fs.c | 33 ++-
src/util/storage_file.c | 31 +
src/util/storage_file.h | 46 ++
src/vbox/vbox_tmpl.c | 15 ++-
9 files changed, 125 insertions(+), 65 deletions(-)
create mode 100644 src/util/storage_file.c
create mode 100644 src/util/storage_file.h
diff --git a/src/Makefile.am b/src/Makefile.am
index 5bddb58..9662fd4 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -50,6 +50,7 @@ UTIL_SOURCES =
\
util/qparams.c util/qparams.h \
util/stats_linux.c util/stats_linux.h \
util/storage_encryption.c util/storage_encryption.h \
+ util/storage_file.c util/storage_file.h \
util/threads.c util/threads.h \
util/threads-pthread.h \
util/threads-win32.h\
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index cb063cc..788de15 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -36,6 +36,7 @@
#include virterror_internal.h
#include datatypes.h
#include storage_conf.h
+#include storage_file.h
#include xml.h
#include uuid.h
@@ -82,12 +83,6 @@ VIR_ENUM_IMPL(virStorageVolFormatDisk,
linux-lvm, linux-raid,
extended)
-VIR_ENUM_IMPL(virStorageVolFormatFileSystem,
- VIR_STORAGE_VOL_FILE_LAST,
- raw, dir, bochs,
- cloop, cow, dmg, iso,
- qcow, qcow2, vmdk, vpc)
-
VIR_ENUM_IMPL(virStoragePartedFsType,
VIR_STORAGE_PARTED_FS_TYPE_LAST,
ext2, ext2, fat16,
@@ -150,9 +145,9 @@ static virStoragePoolTypeInfo poolTypeInfo[] = {
},
{ .poolType = VIR_STORAGE_POOL_DIR,
.volOptions = {
-.defaultFormat = VIR_STORAGE_VOL_FILE_RAW,
-.formatFromString = virStorageVolFormatFileSystemTypeFromString,
-.formatToString = virStorageVolFormatFileSystemTypeToString,
+.defaultFormat = VIR_STORAGE_FILE_RAW,
+.formatFromString = virStorageFileFormatTypeFromString,
+.formatToString = virStorageFileFormatTypeToString,
},
},
{ .poolType = VIR_STORAGE_POOL_FS,
@@ -162,9 +157,9 @@ static virStoragePoolTypeInfo poolTypeInfo[] = {
.formatToString = virStoragePoolFormatFileSystemTypeToString,
},
.volOptions = {
-.defaultFormat = VIR_STORAGE_VOL_FILE_RAW,
-.formatFromString = virStorageVolFormatFileSystemTypeFromString,
-.formatToString = virStorageVolFormatFileSystemTypeToString,
+.defaultFormat = VIR_STORAGE_FILE_RAW,
+.formatFromString = virStorageFileFormatTypeFromString,
+.formatToString = virStorageFileFormatTypeToString,
},
},
{ .poolType = VIR_STORAGE_POOL_NETFS,
@@ -176,9 +171,9 @@ static virStoragePoolTypeInfo poolTypeInfo[] = {
.formatToString = virStoragePoolFormatFileSystemNetTypeToString,
},
.volOptions = {
-.defaultFormat = VIR_STORAGE_VOL_FILE_RAW,
-.formatFromString = virStorageVolFormatFileSystemTypeFromString,
-.formatToString = virStorageVolFormatFileSystemTypeToString,
+.defaultFormat = VIR_STORAGE_FILE_RAW,
+.formatFromString = virStorageFileFormatTypeFromString,
+.formatToString = virStorageFileFormatTypeToString,
},
},
{ .poolType = VIR_STORAGE_POOL_ISCSI,
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index 00dd102..7f63fbc 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -429,23 +429,6 @@ enum virStoragePoolFormatLogical {
};
VIR_ENUM_DECL(virStoragePoolFormatLogical)
-
-enum virStorageVolFormatFileSystem {
-VIR_STORAGE_VOL_FILE_RAW = 0,
-VIR_STORAGE_VOL_FILE_DIR,
-VIR_STORAGE_VOL_FILE_BOCHS,
-VIR_STORAGE_VOL_FILE_CLOOP,
-VIR_STORAGE_VOL_FILE_COW,
-VIR_STORAGE_VOL_FILE_DMG,
-VIR_STORAGE_VOL_FILE_ISO,
-
[libvirt] [PATCH 2/7] Move storage encryption code to src/util
This is so we can use it in the storage file probing code which is also moving to src/util The real code change is renaming virStorageReportError() to virStorageEncryptionReportError() * src/util/storage_encryption.[ch]: add * src/conf/storage_encryption_conf.[ch]: remove * src/conf/domain_conf.h, storage/conf/storage_conf.h: include storage_encryption.h * src/Makefile.am: build src/util/storage_encryption.[ch]; note libvirt_lxc now needs to be linked with $(SELINUX_LIBS) * proxy/Makefile.am: ditto * po/POTFILES.in, src/libvirt_private.syms: update --- po/POTFILES.in |2 +- proxy/Makefile.am |2 +- src/Makefile.am| 52 +++--- src/conf/domain_conf.h |2 +- src/conf/storage_conf.h|2 +- src/conf/storage_encryption_conf.c | 294 --- src/conf/storage_encryption_conf.h | 80 -- src/libvirt_private.syms |2 +- src/util/storage_encryption.c | 301 src/util/storage_encryption.h | 80 ++ 10 files changed, 412 insertions(+), 405 deletions(-) delete mode 100644 src/conf/storage_encryption_conf.c delete mode 100644 src/conf/storage_encryption_conf.h create mode 100644 src/util/storage_encryption.c create mode 100644 src/util/storage_encryption.h diff --git a/po/POTFILES.in b/po/POTFILES.in index 67769c8..9f21459 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -7,7 +7,6 @@ src/conf/network_conf.c src/conf/node_device_conf.c src/conf/secret_conf.c src/conf/storage_conf.c -src/conf/storage_encryption_conf.c src/datatypes.c src/interface/netcf_driver.c src/libvirt.c @@ -45,6 +44,7 @@ src/util/conf.c src/util/iptables.c src/util/logging.c src/util/pci.c +src/util/storage_encryption.c src/util/util.c src/util/uuid.c src/util/virterror.c diff --git a/proxy/Makefile.am b/proxy/Makefile.am index 3e0050b..2ae6ef9 100644 --- a/proxy/Makefile.am +++ b/proxy/Makefile.am @@ -17,12 +17,12 @@ libvirt_proxy_SOURCES = libvirt_proxy.c \ @top_srcdir@/src/util/buf.c \ @top_srcdir@/src/util/logging.c \ @top_srcdir@/src/util/memory.c \ +@top_srcdir@/src/util/storage_encryption.c \ @top_srcdir@/src/util/threads.c \ @top_srcdir@/src/util/util.c \ @top_srcdir@/src/util/uuid.c \ @top_srcdir@/src/util/virterror.c \ @top_srcdir@/src/conf/capabilities.c \ -@top_srcdir@/src/conf/storage_encryption_conf.c \ @top_srcdir@/src/conf/domain_conf.c \ @top_srcdir@/src/xen/xend_internal.c \ @top_srcdir@/src/xen/xen_hypervisor.c \ diff --git a/src/Makefile.am b/src/Makefile.am index 9cbec47..5bddb58 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -35,26 +35,27 @@ mod_LTLIBRARIES = # These files are not related to driver APIs. Simply generic # helper APIs for various purposes -UTIL_SOURCES = \ - util/bridge.c util/bridge.h \ - util/buf.c util/buf.h \ - util/conf.c util/conf.h \ - util/cgroup.c util/cgroup.h \ - util/event.c util/event.h \ - util/hash.c util/hash.h \ - util/iptables.c util/iptables.h \ - util/logging.c util/logging.h \ - util/memory.c util/memory.h \ - util/pci.c util/pci.h \ - util/hostusb.c util/hostusb.h \ - util/qparams.c util/qparams.h \ - util/stats_linux.c util/stats_linux.h \ - util/threads.c util/threads.h \ - util/threads-pthread.h \ - util/threads-win32.h\ - util/uuid.c util/uuid.h \ - util/util.c util/util.h \ - util/xml.c util/xml.h \ +UTIL_SOURCES = \ + util/bridge.c util/bridge.h \ + util/buf.c util/buf.h \ + util/conf.c util/conf.h \ + util/cgroup.c util/cgroup.h \ + util/event.c util/event.h \ + util/hash.c util/hash.h \ + util/iptables.c util/iptables.h \ + util/logging.c util/logging.h \ +
[libvirt] [PATCH 5/7] Move file info functions to libvirt_util
Rename virStorageBackendUpdateVolTargetInfo to virStorageFileGetInfo()
and move to util/storage_file.[ch]
* src/storage/storage_backend.[ch]: move code from here ...
* src/util/storage_file.[ch]: ... to here
* src/libvirt_private.syms: export new functions
* src/storage/storage_backend_fs.c, src/storage/storage_backend_mpath.c,
src/storage/storage_backend_scsi.c: update from above changes
* po/POTFILES.in: add storage_file.c
---
po/POTFILES.in |1 +
src/libvirt_private.syms|2 +
src/storage/storage_backend.c | 148 ++---
src/storage/storage_backend.h |9 --
src/storage/storage_backend_fs.c| 18 ++--
src/storage/storage_backend_mpath.c |7 +-
src/storage/storage_backend_scsi.c |7 +-
src/util/storage_file.c | 157 +++
src/util/storage_file.h | 13 +++
9 files changed, 194 insertions(+), 168 deletions(-)
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 9f21459..30c52f5 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -45,6 +45,7 @@ src/util/iptables.c
src/util/logging.c
src/util/pci.c
src/util/storage_encryption.c
+src/util/storage_file.c
src/util/util.c
src/util/uuid.c
src/util/virterror.c
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 76a6e1b..9b6f4a7 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -393,6 +393,8 @@ virStorageGenerateQcowPassphrase;
# storage_file.h
virStorageFileFormatTypeToString;
virStorageFileFormatTypeFromString;
+virStorageFileGetInfo;
+virStorageFileGetInfoFromFD;
# threads.h
virMutexInit;
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 18a69be..e214c3f 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -73,10 +73,6 @@
#include storage_backend_fs.h
#endif
-#ifndef DEV_BSIZE
-#define DEV_BSIZE 512
-#endif
-
#define VIR_FROM_THIS VIR_FROM_STORAGE
static virStorageBackendPtr backends[] = {
@@ -768,33 +764,6 @@ virStorageBackendForType(int type) {
return NULL;
}
-
-int
-virStorageBackendUpdateVolTargetInfo(virConnectPtr conn,
- virStorageVolTargetPtr target,
- unsigned long long *allocation,
- unsigned long long *capacity)
-{
-int ret, fd;
-
-if ((fd = open(target-path, O_RDONLY)) 0) {
-virReportSystemError(conn, errno,
- _(cannot open volume '%s'),
- target-path);
-return -1;
-}
-
-ret = virStorageBackendUpdateVolTargetInfoFD(conn,
- target,
- fd,
- allocation,
- capacity);
-
-close(fd);
-
-return ret;
-}
-
int
virStorageBackendUpdateVolInfo(virConnectPtr conn,
virStorageVolDefPtr vol,
@@ -802,122 +771,23 @@ virStorageBackendUpdateVolInfo(virConnectPtr conn,
{
int ret;
-if ((ret = virStorageBackendUpdateVolTargetInfo(conn,
-vol-target,
-vol-allocation,
-withCapacity ?
vol-capacity : NULL)) 0)
+if ((ret = virStorageFileGetInfo(conn,
+ vol-target.path,
+ vol-target.perms,
+ vol-allocation,
+ withCapacity ? vol-capacity : NULL))
0)
return ret;
if (vol-backingStore.path
-(ret = virStorageBackendUpdateVolTargetInfo(conn,
-vol-backingStore,
-NULL, NULL)) 0)
+(ret = virStorageFileGetInfo(conn,
+ vol-backingStore.path,
+ vol-backingStore.perms,
+ NULL, NULL)) 0)
return ret;
return 0;
}
-/*
- * virStorageBackendUpdateVolTargetInfoFD:
- * @conn: connection to report errors on
- * @target: target definition ptr of volume to update
- * @fd: fd of storage volume to update
- * @allocation: If not NULL, updated allocation information will be stored
- * @capacity: If not NULL, updated capacity info will be stored
- *
- * Returns 0 for success-1 on a legitimate error condition,
- *-2 if passed FD isn't a regular, char, or block file.
- */
-int
-virStorageBackendUpdateVolTargetInfoFD(virConnectPtr conn,
- virStorageVolTargetPtr target,
- int fd,
- unsigned long long *allocation,
-
[libvirt] [PATCH 7/7] Re-label image file backing stores
Use virStorageFileProbeHeader() to find any backing stores for images
and re-label them
Without this, qemu cannot access qcow2 backing files, see:
https://bugzilla.redhat.com/497131
* src/security/security_selinux.c: re-label backing store files in
SELinuxSetSecurityImageLabel()
---
src/security/security_selinux.c | 26 ++
1 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index b84de8f..f415118 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -27,6 +27,7 @@
#include logging.h
#include pci.h
#include hostusb.h
+#include storage_file.h
#define VIR_FROM_THIS VIR_FROM_SECURITY
@@ -403,10 +404,35 @@ SELinuxSetSecurityImageLabel(virConnectPtr conn,
{
const virSecurityLabelDefPtr secdef = vm-def-seclabel;
+const char *path;
+char *backingStore = NULL;
if (!disk-src)
return 0;
+path = disk-src;
+do {
+int ret;
+
+ret = virStorageFileProbeHeader(conn, path, NULL, backingStore,
+NULL, NULL, NULL, NULL);
+
+if (path != disk-src)
+VIR_FREE(path);
+path = NULL;
+
+if (ret 0)
+return -1;
+
+if (backingStore != NULL
+SELinuxSetFilecon(conn, backingStore, default_content_context)
0) {
+VIR_FREE(backingStore);
+return -1;
+}
+
+path = backingStore;
+} while (path != NULL);
+
if (disk-shared) {
return SELinuxSetFilecon(conn, disk-src, default_image_context);
} else if (disk-readonly) {
--
1.6.2.5
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/7] Move virStoragePerms to libvirt_util
Rename to virStorageFilePerms and move to util/storage_file.[ch]
* src/conf/storage_conf.h: move perms struct from here ...
* src/util/storage_file.h: ... to here
* src/conf/storage_conf.c: update
---
src/conf/storage_conf.c |2 +-
src/conf/storage_conf.h | 18 +++---
src/util/storage_file.h |9 +
3 files changed, 13 insertions(+), 16 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 788de15..dc6171c 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -387,7 +387,7 @@ virStoragePoolDefParseAuthChap(virConnectPtr conn,
static int
virStorageDefParsePerms(virConnectPtr conn,
xmlXPathContextPtr ctxt,
-virStoragePermsPtr perms,
+virStorageFilePermsPtr perms,
const char *permxpath,
int defaultmode) {
char *mode;
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index 7f63fbc..9353cf5 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -27,23 +27,11 @@
#include internal.h
#include util.h
#include storage_encryption.h
+#include storage_file.h
#include threads.h
#include libxml/tree.h
-/* Shared structs */
-
-
-
-typedef struct _virStoragePerms virStoragePerms;
-typedef virStoragePerms *virStoragePermsPtr;
-struct _virStoragePerms {
-int mode;
-int uid;
-int gid;
-char *label;
-};
-
/* Storage volumes */
@@ -76,7 +64,7 @@ typedef virStorageVolTarget *virStorageVolTargetPtr;
struct _virStorageVolTarget {
char *path;
int format;
-virStoragePerms perms;
+virStorageFilePerms perms;
int type; /* only used by disk backend for partition type */
/* Currently used only in virStorageVolDef.target, not in .backingstore. */
virStorageEncryptionPtr encryption;
@@ -236,7 +224,7 @@ typedef struct _virStoragePoolTarget virStoragePoolTarget;
typedef virStoragePoolTarget *virStoragePoolTargetPtr;
struct _virStoragePoolTarget {
char *path;/* Optional local filesystem mapping */
-virStoragePerms perms; /* Default permissions for volumes */
+virStorageFilePerms perms; /* Default permissions for volumes */
};
diff --git a/src/util/storage_file.h b/src/util/storage_file.h
index 7bccbe4..e2f1478 100644
--- a/src/util/storage_file.h
+++ b/src/util/storage_file.h
@@ -43,4 +43,13 @@ enum virStorageFileFormat {
VIR_ENUM_DECL(virStorageFileFormat);
+typedef struct _virStorageFilePerms virStorageFilePerms;
+typedef virStorageFilePerms *virStorageFilePermsPtr;
+struct _virStorageFilePerms {
+int mode;
+int uid;
+int gid;
+char *label;
+};
+
#endif /* __VIR_STORAGE_FILE_H__ */
--
1.6.2.5
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 6/7] Move virStorageBackendProbeTarget to libvirt_util
Finally, we get to the point of all this.
Rename virStorageBackendProbeTarget() to virStorageFileProbeHeader()
and move to src/util/storage_file.[ch]
* src/storage/storage_backend_fs.c: move code from here ...
* src/util/storage_file.[ch]: ... to here
* src/libvirt_private.syms: export virStorageFileProbeHeader()
---
src/libvirt_private.syms |1 +
src/storage/storage_backend_fs.c | 435 ++
src/util/storage_file.c | 409 +++
src/util/storage_file.h | 10 +
4 files changed, 435 insertions(+), 420 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9b6f4a7..6a353ee 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -395,6 +395,7 @@ virStorageFileFormatTypeToString;
virStorageFileFormatTypeFromString;
virStorageFileGetInfo;
virStorageFileGetInfoFromFD;
+virStorageFileProbeHeader;
# threads.h
virMutexInit;
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 49cfc6f..fdc12c3 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -46,419 +46,10 @@
#include memory.h
#include xml.h
-enum lv_endian {
-LV_LITTLE_ENDIAN = 1, /* 1234 */
-LV_BIG_ENDIAN /* 4321 */
-};
-
-enum {
-BACKING_STORE_OK,
-BACKING_STORE_INVALID,
-BACKING_STORE_ERROR,
-};
-
-static int cowGetBackingStore(virConnectPtr, char **,
- const unsigned char *, size_t);
-static int qcowXGetBackingStore(virConnectPtr, char **,
-const unsigned char *, size_t);
-static int vmdk4GetBackingStore(virConnectPtr, char **,
-const unsigned char *, size_t);
-
-/* Either 'magic' or 'extension' *must* be provided */
-struct FileTypeInfo {
-int type; /* One of the constants above */
-const char *magic; /* Optional string of file magic
- * to check at head of file */
-const char *extension; /* Optional file extension to check */
-enum lv_endian endian; /* Endianness of file format */
-int versionOffset;/* Byte offset from start of file
- * where we find version number,
- * -1 to skip version test */
-int versionNumber;/* Version number to validate */
-int sizeOffset; /* Byte offset from start of file
- * where we find capacity info,
- * -1 to use st_size as capacity */
-int sizeBytes;/* Number of bytes for size field */
-int sizeMultiplier; /* A scaling factor if size is not in bytes */
- /* Store a COW base image path (possibly relative),
- * or NULL if there is no COW base image, to RES;
- * return BACKING_STORE_* */
-int qcowCryptOffset; /* Byte offset from start of file
- * where to find encryption mode,
- * -1 if encryption is not used */
-int (*getBackingStore)(virConnectPtr conn, char **res,
- const unsigned char *buf, size_t buf_size);
-};
-struct FileTypeInfo const fileTypeInfo[] = {
-/* Bochs */
-/* XXX Untested
-{ VIR_STORAGE_FILE_BOCHS, Bochs Virtual HD Image, NULL,
- LV_LITTLE_ENDIAN, 64, 0x2,
- 32+16+16+4+4+4+4+4, 8, 1, -1, NULL },*/
-/* CLoop */
-/* XXX Untested
-{ VIR_STORAGE_VOL_CLOOP, #!/bin/sh\n#V2.0 Format\nmodprobe cloop file=$0
mount -r -t iso9660 /dev/cloop $1\n, NULL,
- LV_LITTLE_ENDIAN, -1, 0,
- -1, 0, 0, -1, NULL }, */
-/* Cow */
-{ VIR_STORAGE_FILE_COW, OOOM, NULL,
- LV_BIG_ENDIAN, 4, 2,
- 4+4+1024+4, 8, 1, -1, cowGetBackingStore },
-/* DMG */
-/* XXX QEMU says there's no magic for dmg, but we should check... */
-{ VIR_STORAGE_FILE_DMG, NULL, .dmg,
- 0, -1, 0,
- -1, 0, 0, -1, NULL },
-/* XXX there's probably some magic for iso we can validate too... */
-{ VIR_STORAGE_FILE_ISO, NULL, .iso,
- 0, -1, 0,
- -1, 0, 0, -1, NULL },
-/* Parallels */
-/* XXX Untested
-{ VIR_STORAGE_FILE_PARALLELS, WithoutFreeSpace, NULL,
- LV_LITTLE_ENDIAN, 16, 2,
- 16+4+4+4+4, 4, 512, -1, NULL },
-*/
-/* QCow */
-{ VIR_STORAGE_FILE_QCOW, QFI, NULL,
- LV_BIG_ENDIAN, 4, 1,
- 4+4+8+4+4, 8, 1, 4+4+8+4+4+8+1+1+2, qcowXGetBackingStore },
-/* QCow 2 */
-{ VIR_STORAGE_FILE_QCOW2, QFI, NULL,
- LV_BIG_ENDIAN, 4, 2,
- 4+4+8+4+4, 8, 1, 4+4+8+4+4+8, qcowXGetBackingStore },
-/* VMDK 3 */
-/* XXX Untested
-{ VIR_STORAGE_FILE_VMDK, COWD, NULL,
- LV_LITTLE_ENDIAN, 4, 1,
- 4+4+4, 4, 512, -1, NULL },
-*/
-/* VMDK 4 */
-{ VIR_STORAGE_FILE_VMDK, KDMV, NULL,
- LV_LITTLE_ENDIAN, 4, 1,
- 4+4+4, 8, 512, -1, vmdk4GetBackingStore },
-/* Connectix / VirtualPC */
-/*
Re: [libvirt] [PATCH 3/9] Add new domain device: controller
On Fri, Sep 25, 2009 at 03:03:27PM +0200, Wolfgang Mauerer wrote:
Daniel P. Berrange wrote:
On Fri, Sep 18, 2009 at 05:26:10PM +0200, Wolfgang Mauerer wrote:
This augments virDomainDevice with a controller element
that is used to represent disk controllers (e.g., scsi
controllers). The XML format is given by
controller type=scsi id=my_id
bus addr=Domain:Bus:Slot
/controller
where type denotes the disk interface (scsi, ide,...), id
is an arbitrary string that identifies the controller for
disk hotadd/remove, and bus addr denotes the controller address
on the PCI bus.
The bus element can be omitted; in this case,
an address will be assigned automatically. Currently,
only hotplugging scsi controllers on a PCI bus
is supported, and this only for qemu guests
As mentioned in the previous patch, I reckon 'id' is better
called 'name'.
For PCI addresses, it is desirable to fully normalize the XML
format, by which I mean have separate attributes for domain,
bus and slot. We already have a syntax for PCI addresses used
for host device passthrough, so it'd make sense to use the
same syntax here for controllers. More broadly, we're probably
going to have to add a PCI address element to all our devices.
While it is unlikely we'll need non-PCI addresses, it doesn't
hurt to make it explicit by adding a type='pci' attribute
Thus I'd suggest
address type='pci' domain='0x' bus='0x06' slot='0x12'/
Instead of
bus addr=Domain:Bus:Slot
In the domain_conf.c/.h parser, we could have a datatype like
enum {
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI,
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST
};
typedef struct _virDomainDevicePCIAddress virDomainDevicePCIAddress;
struct _virDomainDevicePCIAddress {
unsigned domain;
unsigned bus;
unsigned slot;
unsigned function;
};
typedef struct _virDomainDeviceAddress virDomainDeviceAddress;
struct _virDomainDeviceAddress {
int type;
union {
virDomainDevicePCIAddress pci;
} data;
};
Which we then use in all the places in domain_conf.h wanting
address information. Obviously we'd not use the 'function'
field in most places, but doesn't hurt to have it.
And a pair of methods for parsing/formatting this address info
we can call from all the appropriate locations.
using a generic format for PCi addresses surely makes sense,
especially wrt. the common data structure and parsing routines -
I'll add these into the next series. However, I'm not sure
if using address type='pci' domain='0x' bus='0x06' slot='0x12'/
instead of a simple string provides much of an advantage
and does not just increase the typing strain (though there are
maybe some small advantages for computerised parsing of libvirt
configuration files by other apps).
I think its valuable to have the consistent representation across the
different areas of the XML, and although its obviously more verbose,
it is beneficial for applications to only need an XML parser and not
have to then write further parsers for attribute values.
There's already a generic definition of PCI addresses in
docs/schemas/domain.rng:\approx 1140, I suppose it's your
intention to extend if by a type field?
Yes, prettty much
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 :|
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix API doc extractor to stop munging comment formatting
On Fri, Sep 25, 2009 at 01:28:51PM +0100, Daniel P. Berrange wrote: The python method help docs are copied across from the C funtion comments, but in the process all line breaks and indentation was being lost. This made the resulting text and code examples completely unreadable. Both the API doc extractor and the python generator were destroying whitespace this fixes them to preserve it exactly. * docs/apibuild.py: Preserve all whitespace when extracting function comments. Print function comment inside a ![CDATA[ section to fully preserve all whitespace. Look for the word 'returns' to describe return values, instead of 'return' to avoid getting confused with code examples including the C 'return' statement. * python/generator.py: Preserve all whitespace when printing function help docs * src/libvirt.c: Change any return parameter indicated by 'return' to be 'returns', to avoid confusing the API extractor [...] if desc != None and desc != : - output.write( info%s/info\n % (escape(desc))) + output.write( info![CDATA[%s]]/info\n % (desc)) Ah, okay you're using CDATA ! self.indexString(name, desc) for arg in args: (name, desc) = arg @@ -1760,7 +1771,7 @@ class docBuilder: try: desc = id.extra if desc != None and desc != : - output.write(\n info%s/info\n % (escape(desc))) + output.write(\n info![CDATA[%s]]/info\n % (desc)) output.write(/typedef\n) else: output.write(/\n) @@ -1796,7 +1807,7 @@ class docBuilder: output.write( cond%s/cond\n% (apstr)); try: (ret, params, desc) = id.info - output.write( info%s/info\n % (escape(desc))) + output.write( info![CDATA[%s]]/info\n % (desc)) self.indexString(name, desc) if ret[0] != None: if ret[0] == void: diff --git a/python/generator.py b/python/generator.py index c34cb34..178a415 100755 --- a/python/generator.py +++ b/python/generator.py @@ -44,6 +44,7 @@ if sgmlop: self.finish_starttag = target.start self.finish_endtag = target.end self.handle_data = target.data +self.handle_cdata = target.cdata # activate parser self.parser = sgmlop.XMLParser() @@ -78,6 +79,7 @@ class SlowParser(xmllib.XMLParser): def __init__(self, target): self.unknown_starttag = target.start self.handle_data = target.data +self.handle_cdata = target.cdata self.unknown_endtag = target.end xmllib.XMLParser.__init__(self) @@ -108,6 +110,11 @@ class docParser: print data %s % text self._data.append(text) +def cdata(self, text): +if debug: +print data %s % text +self._data.append(text) + def start(self, tag, attrs): Okay makes sense, plus the large amount of return - Returns changes ACK ! I'm still waiting to see what this gives in the HTML docs :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix API doc extractor to stop munging comment formatting
On Fri, Sep 25, 2009 at 03:33:41PM +0200, Daniel Veillard wrote: On Fri, Sep 25, 2009 at 01:28:51PM +0100, Daniel P. Berrange wrote: The python method help docs are copied across from the C funtion comments, but in the process all line breaks and indentation was being lost. This made the resulting text and code examples completely unreadable. Both the API doc extractor and the python generator were destroying whitespace this fixes them to preserve it exactly. * docs/apibuild.py: Preserve all whitespace when extracting function comments. Print function comment inside a ![CDATA[ section to fully preserve all whitespace. Look for the word 'returns' to describe return values, instead of 'return' to avoid getting confused with code examples including the C 'return' statement. * python/generator.py: Preserve all whitespace when printing function help docs * src/libvirt.c: Change any return parameter indicated by 'return' to be 'returns', to avoid confusing the API extractor [...] if desc != None and desc != : - output.write( info%s/info\n % (escape(desc))) + output.write( info![CDATA[%s]]/info\n % (desc)) Ah, okay you're using CDATA ! self.indexString(name, desc) for arg in args: (name, desc) = arg @@ -1760,7 +1771,7 @@ class docBuilder: try: desc = id.extra if desc != None and desc != : - output.write(\n info%s/info\n % (escape(desc))) + output.write(\n info![CDATA[%s]]/info\n % (desc)) output.write(/typedef\n) else: output.write(/\n) @@ -1796,7 +1807,7 @@ class docBuilder: output.write( cond%s/cond\n% (apstr)); try: (ret, params, desc) = id.info - output.write( info%s/info\n % (escape(desc))) + output.write( info![CDATA[%s]]/info\n % (desc)) self.indexString(name, desc) if ret[0] != None: if ret[0] == void: diff --git a/python/generator.py b/python/generator.py index c34cb34..178a415 100755 --- a/python/generator.py +++ b/python/generator.py @@ -44,6 +44,7 @@ if sgmlop: self.finish_starttag = target.start self.finish_endtag = target.end self.handle_data = target.data +self.handle_cdata = target.cdata # activate parser self.parser = sgmlop.XMLParser() @@ -78,6 +79,7 @@ class SlowParser(xmllib.XMLParser): def __init__(self, target): self.unknown_starttag = target.start self.handle_data = target.data +self.handle_cdata = target.cdata self.unknown_endtag = target.end xmllib.XMLParser.__init__(self) @@ -108,6 +110,11 @@ class docParser: print data %s % text self._data.append(text) +def cdata(self, text): +if debug: +print data %s % text +self._data.append(text) + def start(self, tag, attrs): Okay makes sense, plus the large amount of return - Returns changes ACK ! I'm still waiting to see what this gives in the HTML docs :-) Hmm, I didn't check that yet, but I guess it shouldn't make any difference, since the HTML will still collapse the whitespace as before - we'd need to insert real p breaks use pre during generation of the HTML if we wanted to maintain any kind of sane formatting 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/11] Add public API definition for data stream handling
On Fri, Sep 25, 2009 at 02:09:54PM +0100, Daniel P. Berrange wrote: On Fri, Sep 25, 2009 at 02:32:39PM +0200, Daniel Veillard wrote: Hum, this would then raise the signal that stream can be used both ways, do we really want to suggest this at the API level, I can see how we're gonna use this internally, but aren't we opening the door to much complexity ? Yeah, that's more or less why I left it out so far - I've not yet found a case where I absolutely needed the WRITE/READ flags to be set explicitly by apps. Okay, let's keep it that way. Ultimately we could add another API to create the Stream. If we wanted to mandate use of READ/WRITE flags for stream creation, we'd obviously need todo it from teh start, since we couldn't add that as a mandatory flag once the API is released in use by apps. yes that's a good point, a design issue too. If you really expect API usage for both read and write, then I would say we should make those flags mandatory. The only point is that our existing flags use in APIs are just fine with 0 as being the 'default', and we would break this but it's not a big deal IMHO, that will be caught immediately. There is one likely API where we'd have a full read+write stream. I've thought about adding ability to tunnel a serial port PTY over libvirt, so 'virsh console' could be made to work remotely. eg, something like virDomainOpenConsole(virDomainPtr dom, virStreamPtr stream const char *consolename); In this case you'd be reading writing from /to the same stream. It still wouldn't really require that we set the READ+WRITE flags when doing virStreamNew() okay +typedef int (*virStreamSinkFunc)(virStreamPtr st, + const char *data, + size_t nbytes, + void *opaque); Same thing do we allow a sink function to be called repeatedly ? If we want to allow this in some ways we will need an extra argument to indicate the end of the stream. Even if we don't plan this yet, I would suggest to add a flags to allow for this possibility in the future. With a chunk size of 256K at the protocol level it may not be a good idea to keep the full data in memory, so I would allow for this interface to call the sink multiple times. And IMHO it's best to pass the indication of end of transfer directly at the sink level rather than wait for the virStreamFree() coming from the user. +int virStreamRecvAll(virStreamPtr st, + virStreamSinkFunc handler, + void *opaque); Okay Same as for SendAll, this API will invoke the handler multilpe times to write out data that is being received. In both cases the implementation is invoking the handler with 64kb buffers to avoid pulling lots of data into memory. Okay, but I think being able to indicate there that a packet is the last one may be important, for example if the application design prefer to initiate the closure of the transfer (close/sync/...) as soon as possible. Actually in the case of the 'source' function, the app already knows when its got to the end, because its source funtion will be returning '0' for end-of-file. yes for read it's not a problem For the 'sink' function we'd have to make sure we called it once at the end with a length of '0' to indicate EOF in that direction. I can't remember offhand if we'll do that already or not. ah write 0 lenght, yes that's another way to pass the signal. Also how flexible are we in the design with callbacks taling a long time to complete, for example reads crossing the network, or slow output devices ? Maybe this should be hinted in the callback descriptions. These callbacks are the app's responsibility execute in its context, so libvirt doesn't care whether they are slow or fast to execute. Everything internal to libvirt relating to streams is non-blocking fast. okidoc, I just wanted to doub;e-check :-) The sink/source callbacks do not need to close the FD since this is easily done with RecvAll/SendAll returns control to the application. THis is in fact important, because it is not until RecvAll/SendAll returns that you can call virStreamFinish to check for success. If it did not suceed then you may want do other cleanup before closing the FD, such as unlinking the file Hum, the two example for RecvAll and SendAll don't suggest virStreamFinish() to be called to get the status, I would expect error reporting to show up as the result code from RecvAll and SendAll. Actually these 2 code examples are wrong. There should be a call to virStreamFinish in there, before the virStreamFree. This was not required in an earlier version of my patch, because SendAll would call Finish for you, but I realized this made
Re: [libvirt] [PATCH] Support a new peer-to-peer migration mode public API
On Fri, Sep 25, 2009 at 02:27:23PM +0100, Daniel P. Berrange wrote:
On Tue, Sep 22, 2009 at 02:43:20PM +0200, Daniel Veillard wrote:
[ Sending again due to mail output problems ]
On Thu, Sep 17, 2009 at 06:25:01PM +0100, Daniel P. Berrange wrote:
Introduces several new public API options for migration
- VIR_MIGRATE_PEER2PEER: With this flag the client only
invokes the virDomainMigratePerform method, expecting
the source host driver to do whatever is required to
complete the entire migration process.
- VIR_MIGRATE_TUNNELLED: With this flag the actual data
for migration will be tunnelled over the libvirtd RPC
channel. This requires that VIR_MIGRATE_PEER2PEER is
also set.
Hum, I would rather add that when entering libvirt.c entry point
set explicitely VIR_MIGRATE_PEER2PEER if VIR_MIGRATE_TUNNELLED is asked
for, that's an internal impl. detail.
I didn't do that, because I wanted to leave open the option
to supporting TUNNELLED mode, without PEER2PEER mode. I
don't really know what that would look like, but I think it
is better future proofing to not presume one implies the other.
Okay, let's go for this.
- virDomainMigrateToURI: This is variant of the existing
virDomainMigrate method which can be used when the
VIR_MIGRATE_PEER2PEER flag is set. The benefit of this
method is that no virConnectPtr object is required for
the destination host, only a destination URI.
Sounds good if we have proper error handling. I'm also wondering
how authentification and access control can be handled in that mode,
the normal method of access somehow garantee that the user credentials
can be checked in some ways, but if it's direct daemon to daemon
talking how can this be performed ?
The client application authenticates with the source libvirtd
in the normal manner. The source libvirtd will authenticate
with the destination libvirtd directly.
The client application will not be authenticating with the
destination libvirtd itself.
While this may sound like a security issue, I don't think it
really is. It is more of an authorization problem - eg the
admin needs to declare somewhere which libvirtd instances
are allowed to migrate guests between then. This authorization
problem is really outside the scope of the client application
or our public app API
Okay, as soon as you want to allow automatic migrations that
will be considered normal.
+int virDomainMigrateToURI (virDomainPtr domain, const char *duri,
+ unsigned long flags, const char *dname,
+ unsigned long bandwidth);
+
s/duri/dest/ or target, the fact it's a connection URI should be made
obvious in the function comment.
Yep, the function comment later only clearly describes what URIs are
acceptable.
[...]
+} else {
+if (flags VIR_MIGRATE_TUNNELLED) {
+virLibConnError(domain-conn, VIR_ERR_OPERATION_INVALID,
+_(cannot perform tunnelled migration
without using peer2peer flag));
+goto error;
+}
I don't really agree, we should just or the flag and hide this from
the API user.
I really did want to leave open the option of relaxing this restriction
in the future, and thus didn't want to automatically set the PEER2PEER
flag - once its set we can never unset it in the future.
+
+
/*
* Not for public use. This function is part of the internal
* implementation of migration in the remote case.
diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h
index 5913798..5f1a7fe 100644
--- a/src/libvirt_internal.h
+++ b/src/libvirt_internal.h
@@ -17,6 +17,7 @@
* License along with this library; if not, write to the Free Software
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
USA
*
+ * NB This file is (secret) ABI sensitive. Append only
Huh ???
The methods here are encoded in the wire protocol ABI,
but not included in the public libvirt.h header. Hence
they are ABI sensitive, but secret thus append only
like the rest of our API
diff --git a/src/xen_unified.c b/src/xen_unified.c
index dfa9ca5..954b187 100644
--- a/src/xen_unified.c
+++ b/src/xen_unified.c
@@ -455,8 +455,11 @@ static int
xenUnifiedSupportsFeature (virConnectPtr conn ATTRIBUTE_UNUSED, int
feature)
{
switch (feature) {
-case VIR_DRV_FEATURE_MIGRATION_V1: return 1;
-default: return 0;
+case VIR_DRV_FEATURE_MIGRATION_V1:
+case VIR_DRV_FEATURE_MIGRATION_P2P:
+return 1;
+default:
+return 0;
}
}
diff --git a/src/xend_internal.c b/src/xend_internal.c
index 7f55116..da5c039 100644
--- a/src/xend_internal.c
+++ b/src/xend_internal.c
@@ -4400,6 +4400,12 @@
Re: [libvirt] [PATCH] Fix API doc extractor to stop munging comment formatting
On Fri, Sep 25, 2009 at 02:42:47PM +0100, Daniel P. Berrange wrote: On Fri, Sep 25, 2009 at 03:33:41PM +0200, Daniel Veillard wrote: On Fri, Sep 25, 2009 at 01:28:51PM +0100, Daniel P. Berrange wrote: I'm still waiting to see what this gives in the HTML docs :-) Hmm, I didn't check that yet, but I guess it shouldn't make any difference, since the HTML will still collapse the whitespace as before - we'd need to insert real p breaks use pre during generation of the HTML if we wanted to maintain any kind of sane formatting it can be a bit nasty, plus there is some xslt processing :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/7] util.h needs libvirt.h for virConnectPtr
On Fri, Sep 25, 2009 at 02:27:27PM +0100, Mark McLoughlin wrote: Seems standard to include internal.h in order to pull in libvirt.h * src/util/util.h: include internal.h --- src/util/util.h |1 + 1 files changed, 1 insertions(+), 0 deletions(-) There don't appear to be any compile issues without this patch -it'll be pulled in already via one of the many other headers it includes 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/7] Move storage encryption code to src/util
On Fri, Sep 25, 2009 at 02:27:28PM +0100, Mark McLoughlin wrote: This is so we can use it in the storage file probing code which is also moving to src/util The real code change is renaming virStorageReportError() to virStorageEncryptionReportError() * src/util/storage_encryption.[ch]: add * src/conf/storage_encryption_conf.[ch]: remove * src/conf/domain_conf.h, storage/conf/storage_conf.h: include storage_encryption.h * src/Makefile.am: build src/util/storage_encryption.[ch]; note libvirt_lxc now needs to be linked with $(SELINUX_LIBS) * proxy/Makefile.am: ditto * po/POTFILES.in, src/libvirt_private.syms: update --- po/POTFILES.in |2 +- proxy/Makefile.am |2 +- src/Makefile.am| 52 +++--- src/conf/domain_conf.h |2 +- src/conf/storage_conf.h|2 +- src/conf/storage_encryption_conf.c | 294 --- src/conf/storage_encryption_conf.h | 80 -- src/libvirt_private.syms |2 +- src/util/storage_encryption.c | 301 src/util/storage_encryption.h | 80 ++ NACK, this is really not very desirable - it was intended that there would be a strict separation between the XML handling code, and general purpose utility functions, so I'm against moving XML handling code into util. I understand why you did this though - the code in storage_backend_fs.c that you later move into util/ depends on it. I think we should refactor the latter though, rather than moving this XML handling code 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/7] Move file format enum to libvirt_util
On Fri, Sep 25, 2009 at 02:27:29PM +0100, Mark McLoughlin wrote: Renam virStorageVolFormatFileSystem to virStorageFileFormat and move to src/util/storage_file.[ch] * src/Makefile.am: add src/util/storage_file.[ch] * src/conf/storage_conf.[ch]: move enum from here ... * src/util/storage_file.[ch]: .. to here * src/libvirt_private.syms: update To/FromString exports * src/storage/storage_backend.c, src/storage/storage_backend_fs.c, src/vbox/vbox_tmpl.c: update for above changes --- src/Makefile.am |1 + src/conf/storage_conf.c | 25 src/conf/storage_conf.h | 17 -- src/libvirt_private.syms |5 ++- src/storage/storage_backend.c| 17 +++-- src/storage/storage_backend_fs.c | 33 ++- src/util/storage_file.c | 31 + src/util/storage_file.h | 46 ++ src/vbox/vbox_tmpl.c | 15 ++- 9 files changed, 125 insertions(+), 65 deletions(-) create mode 100644 src/util/storage_file.c create mode 100644 src/util/storage_file.h ACK, this is fine 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/7] Move virStoragePerms to libvirt_util
On Fri, Sep 25, 2009 at 02:27:30PM +0100, Mark McLoughlin wrote: Rename to virStorageFilePerms and move to util/storage_file.[ch] * src/conf/storage_conf.h: move perms struct from here ... * src/util/storage_file.h: ... to here * src/conf/storage_conf.c: update --- src/conf/storage_conf.c |2 +- src/conf/storage_conf.h | 18 +++--- src/util/storage_file.h |9 + 3 files changed, 13 insertions(+), 16 deletions(-) NACK, i think this is redundant if we re-factor the file probing code just a little. 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/7] Move virStorageBackendProbeTarget to libvirt_util
On Fri, Sep 25, 2009 at 02:27:32PM +0100, Mark McLoughlin wrote:
+int virStorageFileProbeHeader(virConnectPtr conn,
+ const char *path,
+ int *format,
+ char **backingStore,
+ virStorageEncryptionPtr *encryption,
+ virStorageFilePermsPtr perms,
+ unsigned long long *allocation,
+ unsigned long long *capacity);
+
This function prototype is the root cause of most of my objections
earlier in the series.
The way it has evolved over time since I first wrote it has resulted
in sub-optimal separation of responsibilities. In particular I don't
think this method should be responsible for populating the
virStorageEncryptionPtr or virStorageFilePermsPtr structures. It
should focus itself to only be concerned with extracting metadata
from the file header. The callers can later populate the higher
level structures like virStorageFilePermsPtr virStorageEncryptionPtr
as they see fit.
For an API in the util/ library I think it'd be reasonable to aim for
an simpler API such as
int virStorageFileGetMetadata(virConnectPtr conn,
const char *path,
int *format,
char **backingStore,
boolean *encrypted
unsigned long long *allocation,
unsigned long long *capacity);
The backingStore, encrypted, allocation capacity fields would only be
filed in for format != VIR_STORAGE_FILE_RAW. There's probably enough
return parameters here to justify inventing a small (caller allocated)
struct for the returned metadata, such as
typedef struct _virStorageFileMetadata {
int format;
char *backingStore;
boolean encrypted;
unsigned long long allocation;
unsigned long long capacty;
} virStorageFileMetadata;
So callers would do
virStorageFileMetadata meta;
memset(meta, 0, sizeof meta);
virStorageFileGetMetadata(conn, path, meta);
The original virStorageBackendProbeTarget() method in storage_backend_fs.c
could then be written in terms of a combination of calls to the
virStorageFileGetMetadata() and virStorageBackendUpdateVolTargetInfo()
methods. This would avoid the need to move most of the code in this
series.
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 :|
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/7] Move file info functions to libvirt_util
On Fri, Sep 25, 2009 at 02:27:31PM +0100, Mark McLoughlin wrote:
Rename virStorageBackendUpdateVolTargetInfo to virStorageFileGetInfo()
and move to util/storage_file.[ch]
* src/storage/storage_backend.[ch]: move code from here ...
* src/util/storage_file.[ch]: ... to here
* src/libvirt_private.syms: export new functions
* src/storage/storage_backend_fs.c, src/storage/storage_backend_mpath.c,
src/storage/storage_backend_scsi.c: update from above changes
* po/POTFILES.in: add storage_file.c
---
po/POTFILES.in |1 +
src/libvirt_private.syms|2 +
src/storage/storage_backend.c | 148 ++---
src/storage/storage_backend.h |9 --
src/storage/storage_backend_fs.c| 18 ++--
src/storage/storage_backend_mpath.c |7 +-
src/storage/storage_backend_scsi.c |7 +-
src/util/storage_file.c | 157
+++
src/util/storage_file.h | 13 +++
9 files changed, 194 insertions(+), 168 deletions(-)
NACK to this one too, based suggestion against patch 6
-
-target-perms.mode = sb.st_mode S_IRWXUGO;
-target-perms.uid = sb.st_uid;
-target-perms.gid = sb.st_gid;
-
-VIR_FREE(target-perms.label);
-
-#if HAVE_SELINUX
-/* XXX: make this a security driver call */
-if (fgetfilecon(fd, filecon) == -1) {
-if (errno != ENODATA errno != ENOTSUP) {
-virReportSystemError(conn, errno,
- _(cannot get file context of '%s'),
- target-path);
-return -1;
-} else {
-target-perms.label = NULL;
-}
-} else {
-target-perms.label = strdup(filecon);
-if (target-perms.label == NULL) {
-virReportOOMError(conn);
-return -1;
-}
-freecon(filecon);
-}
-#else
-target-perms.label = NULL;
-#endif
This bit of code / todo item is another good argument against moving it - we
need to eventually mak this call into the security driver, and don't want to
have to move the security drivers into the src/util/ directory / library too.
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 :|
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/7] Re-label image file backing stores
On Fri, Sep 25, 2009 at 02:27:33PM +0100, Mark McLoughlin wrote:
Use virStorageFileProbeHeader() to find any backing stores for images
and re-label them
Without this, qemu cannot access qcow2 backing files, see:
https://bugzilla.redhat.com/497131
* src/security/security_selinux.c: re-label backing store files in
SELinuxSetSecurityImageLabel()
---
src/security/security_selinux.c | 26 ++
1 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index b84de8f..f415118 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -27,6 +27,7 @@
#include logging.h
#include pci.h
#include hostusb.h
+#include storage_file.h
#define VIR_FROM_THIS VIR_FROM_SECURITY
@@ -403,10 +404,35 @@ SELinuxSetSecurityImageLabel(virConnectPtr conn,
{
const virSecurityLabelDefPtr secdef = vm-def-seclabel;
+const char *path;
+char *backingStore = NULL;
if (!disk-src)
return 0;
+path = disk-src;
+do {
+int ret;
+
+ret = virStorageFileProbeHeader(conn, path, NULL, backingStore,
+NULL, NULL, NULL, NULL);
+
+if (path != disk-src)
+VIR_FREE(path);
+path = NULL;
+
+if (ret 0)
+return -1;
+
+if (backingStore != NULL
+SELinuxSetFilecon(conn, backingStore, default_content_context)
0) {
+VIR_FREE(backingStore);
+return -1;
+}
+
+path = backingStore;
+} while (path != NULL);
+
if (disk-shared) {
return SELinuxSetFilecon(conn, disk-src, default_image_context);
} else if (disk-readonly) {
ACK, to the principal of this patch, though obviously suggested changes
earlier in the series would impact the actual code a little.
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 :|
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC] Multi-IQN proposal
Would this proposal be acceptable ?
Example XML schema for an iSCSI storage pool created --
pool type=iscsi
namedell/name
uuidcf354733-b01b-8870-2040-94888640e24d/uuid
capacity0/capacity
allocation0/allocation
available0/available
- source
initiator iqnname = initiator IQN1
initiator iqnname = initiator IQN2
host name=iSCSI target hostname or Target IP address /
device path=iSCSI Target IQN name /
/source
- target
path/dev/disk/by-path/path
- permissions
mode0700/mode
owner0/owner
group0/group
/permissions
/target
/pool
Each initiator iqn name can be parsed to create the unique sessions.
This should solve the following possibilities --
* possibility of multiple IQNs for a single Guest
* option for Guest's own BIOS initiator to use these IQNs (iSCSI in
Guest)
* option for hypervisor's initiator to use these IQNs on behalf of the
guest
(Multi-IQN)
Compile tested only. Needs beatification.
diff --git a/src/storage_conf.c b/src/storage_conf.c
index cb063cc..915e897 100644
--- a/src/storage_conf.c
+++ b/src/storage_conf.c
@@ -116,6 +116,7 @@ enum {
VIR_STORAGE_POOL_SOURCE_DIR = (12),
VIR_STORAGE_POOL_SOURCE_ADAPTER = (13),
VIR_STORAGE_POOL_SOURCE_NAME= (14),
+VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN = (15),
};
@@ -537,6 +538,33 @@ virStoragePoolDefParseXML(virConnectPtr conn,
goto cleanup;
}
}
+
+/* Read the conf here */
+if (options-flags VIR_STORAGE_POOL_SOURCE_INITATOR_IQN) {
+ int niqn, i;
+
+ if ((niqn = virXPathNodeSet(conn, ./source/host/initiator,
ctxt, nodeset)) 0) {
+virStorageReportError(conn, VIR_ERR_XML_ERROR,
+%s, _(cannot extract storage pool source
devices));
+goto cleanup;
+}
+ if (VIR_ALLOC_N(ret-source.niqn, niqn) 0) {
+VIR_FREE(nodeset);
+virReportOOMError(conn);
+goto cleanup;
+}
+ for (i = 0 ; i niqn ; i++ ) {
+ xmlChar *name = xmlGetProp(nodeset[i], BAD_CAST iqnname);
+if (path == NULL) {
+VIR_FREE(nodeset);
+virStorageReportError(conn, VIR_ERR_XML_ERROR,
+%s, _(missing storage pool source device
path));
+goto cleanup;
+}
+ ret-source.initiator[i].iqnname = (char *)name;
+ }
+}
if (options-flags VIR_STORAGE_POOL_SOURCE_DEVICE) {
xmlNodePtr *nodeset = NULL;
int nsource, i;
diff --git a/src/storage_conf.h b/src/storage_conf.h
index 421d305..c2504be 100644
--- a/src/storage_conf.h
+++ b/src/storage_conf.h
@@ -202,7 +202,12 @@ struct _virStoragePoolSourceDevice {
} geometry;
};
-
+/* Initiator Attributes */
+typedef virStoragePoolSourceInitiatorAttr
*virStoragePoolSourceInitiatorAttrPtr;
+struct virStoragePoolSourceInitiatorAttr {
+char *iqnname;
+/* We could put other initiator specific things here */
+}
typedef struct _virStoragePoolSource virStoragePoolSource;
typedef virStoragePoolSource *virStoragePoolSourcePtr;
@@ -214,6 +219,9 @@ struct _virStoragePoolSource {
int ndevice;
virStoragePoolSourceDevicePtr devices;
+/*And either one or more initiators*/
+virStoragePoolSourceInitiatorAttrPtr initiator;
+
/* Or a directory */
char *dir;
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] RE: [RFC] Multi-IQN proposal
Do you also need to specify the expected mapping of initiator IQN to use
for each target IQN, or is that not necessary?
--
Matt Domsch
Technology Strategist, Dell Office of the CTO
linux.dell.com www.dell.com/linux
-Original Message-
From: Iyer, Shyam
Sent: Friday, September 25, 2009 2:15 PM
To: libvir-list@redhat.com
Cc: Domsch, Matt; Bellad, Sudhir; KM, Paniraja
Subject: [RFC] Multi-IQN proposal
Would this proposal be acceptable ?
Example XML schema for an iSCSI storage pool created --
pool type=iscsi
namedell/name
uuidcf354733-b01b-8870-2040-94888640e24d/uuid
capacity0/capacity
allocation0/allocation
available0/available
- source
initiator iqnname = initiator IQN1
initiator iqnname = initiator IQN2
host name=iSCSI target hostname or Target IP address /
device path=iSCSI Target IQN name /
/source
- target
path/dev/disk/by-path/path
- permissions
mode0700/mode
owner0/owner
group0/group
/permissions
/target
/pool
Each initiator iqn name can be parsed to create the unique sessions.
This should solve the following possibilities --
* possibility of multiple IQNs for a single Guest
* option for Guest's own BIOS initiator to use these IQNs (iSCSI in
Guest)
* option for hypervisor's initiator to use these IQNs on behalf of the
guest
(Multi-IQN)
Compile tested only. Needs beatification.
diff --git a/src/storage_conf.c b/src/storage_conf.c
index cb063cc..915e897 100644
--- a/src/storage_conf.c
+++ b/src/storage_conf.c
@@ -116,6 +116,7 @@ enum {
VIR_STORAGE_POOL_SOURCE_DIR = (12),
VIR_STORAGE_POOL_SOURCE_ADAPTER = (13),
VIR_STORAGE_POOL_SOURCE_NAME= (14),
+VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN = (15),
};
@@ -537,6 +538,33 @@ virStoragePoolDefParseXML(virConnectPtr conn,
goto cleanup;
}
}
+
+/* Read the conf here */
+if (options-flags VIR_STORAGE_POOL_SOURCE_INITATOR_IQN) {
+ int niqn, i;
+
+ if ((niqn = virXPathNodeSet(conn, ./source/host/initiator,
ctxt, nodeset)) 0) {
+virStorageReportError(conn, VIR_ERR_XML_ERROR,
+%s, _(cannot extract storage pool source
devices));
+goto cleanup;
+}
+ if (VIR_ALLOC_N(ret-source.niqn, niqn) 0) {
+VIR_FREE(nodeset);
+virReportOOMError(conn);
+goto cleanup;
+}
+ for (i = 0 ; i niqn ; i++ ) {
+ xmlChar *name = xmlGetProp(nodeset[i], BAD_CAST iqnname);
+if (path == NULL) {
+VIR_FREE(nodeset);
+virStorageReportError(conn, VIR_ERR_XML_ERROR,
+%s, _(missing storage pool source device
path));
+goto cleanup;
+}
+ ret-source.initiator[i].iqnname = (char *)name;
+ }
+}
if (options-flags VIR_STORAGE_POOL_SOURCE_DEVICE) {
xmlNodePtr *nodeset = NULL;
int nsource, i;
diff --git a/src/storage_conf.h b/src/storage_conf.h
index 421d305..c2504be 100644
--- a/src/storage_conf.h
+++ b/src/storage_conf.h
@@ -202,7 +202,12 @@ struct _virStoragePoolSourceDevice {
} geometry;
};
-
+/* Initiator Attributes */
+typedef virStoragePoolSourceInitiatorAttr
*virStoragePoolSourceInitiatorAttrPtr;
+struct virStoragePoolSourceInitiatorAttr {
+char *iqnname;
+/* We could put other initiator specific things here */
+}
typedef struct _virStoragePoolSource virStoragePoolSource;
typedef virStoragePoolSource *virStoragePoolSourcePtr;
@@ -214,6 +219,9 @@ struct _virStoragePoolSource {
int ndevice;
virStoragePoolSourceDevicePtr devices;
+/*And either one or more initiators*/
+virStoragePoolSourceInitiatorAttrPtr initiator;
+
/* Or a directory */
char *dir;
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] RE: [RFC] Multi-IQN proposal
-Original Message- From: Domsch, Matt Sent: Saturday, September 26, 2009 1:01 AM To: Iyer, Shyam; 'libvir-list@redhat.com' Cc: Bellad, Sudhir; KM, Paniraja Subject: RE: [RFC] Multi-IQN proposal Do you also need to specify the expected mapping of initiator IQN to use for each target IQN, or is that not necessary? That would not be necessary. The targets can do the mapping easily. Most targets today support initiator iqn based mapping. -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] Multi-IQN proposal
shyam_i...@dell.com wrote:
Would this proposal be acceptable ?
In principle, I think what you're proposing is reasonable, and is
certainly contemplated by the iSCSI specs.
Example XML schema for an iSCSI storage pool created --
pool type=iscsi
namedell/name
uuidcf354733-b01b-8870-2040-94888640e24d/uuid
capacity0/capacity
allocation0/allocation
available0/available
- source
initiator iqnname = initiator IQN1
initiator iqnname = initiator IQN2
host name=iSCSI target hostname or Target IP address /
device path=iSCSI Target IQN name /
/source
- target
path/dev/disk/by-path/path
- permissions
mode0700/mode
owner0/owner
group0/group
/permissions
/target
/pool
I think you have the initiator name specified in the right place in the
XML. I would make the initiator iqn an element rather than an
attribute, since your proposal contemplates adding additional initiator
specific information later, and stylistically I think elements will be
cleaner. That gives:
initiator
iqniqn.foo1.bar.baz/iqn
iqniqn.foo2.bar.baz/iqn
iqniqn.foo3.bar.baz/iqn
/initiator
Each initiator iqn name can be parsed to create the unique sessions.
You should propose specifically how you see the sessions being set up.
Each pool currently describes something that basically resembles a
session, so your proposal modifies that paradigm a bit. Another
possible way to implement what you describe would be to allow zero or
one initiator tags within a pool. If no initiator tag is specified, the
system will use the system default; if a tag is specified, the system
will attempt to use the information contained in it. The more I think
about it, the more I like that approach since it keeps the pool paradigm
unmodified.
This should solve the following possibilities --
* possibility of multiple IQNs for a single Guest
* option for Guest's own BIOS initiator to use these IQNs (iSCSI in
Guest)
* option for hypervisor's initiator to use these IQNs on behalf of the
guest
(Multi-IQN)
How is this different from the first possibility?
Compile tested only. Needs beatification.
I didn't go over the code closely, but I didn't see anything that struck
me as completely off base. I think it's more important to get the
details of how this information will be used worked out at this point
than to get the code finalized.
Dave
diff --git a/src/storage_conf.c b/src/storage_conf.c
index cb063cc..915e897 100644
--- a/src/storage_conf.c
+++ b/src/storage_conf.c
@@ -116,6 +116,7 @@ enum {
VIR_STORAGE_POOL_SOURCE_DIR = (12),
VIR_STORAGE_POOL_SOURCE_ADAPTER = (13),
VIR_STORAGE_POOL_SOURCE_NAME= (14),
+VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN = (15),
};
@@ -537,6 +538,33 @@ virStoragePoolDefParseXML(virConnectPtr conn,
goto cleanup;
}
}
+
+/* Read the conf here */
+if (options-flags VIR_STORAGE_POOL_SOURCE_INITATOR_IQN) {
+ int niqn, i;
+
+ if ((niqn = virXPathNodeSet(conn, ./source/host/initiator,
ctxt, nodeset)) 0) {
+virStorageReportError(conn, VIR_ERR_XML_ERROR,
+%s, _(cannot extract storage pool source
devices));
+goto cleanup;
+}
+ if (VIR_ALLOC_N(ret-source.niqn, niqn) 0) {
+VIR_FREE(nodeset);
+virReportOOMError(conn);
+goto cleanup;
+}
+ for (i = 0 ; i niqn ; i++ ) {
+ xmlChar *name = xmlGetProp(nodeset[i], BAD_CAST iqnname);
+if (path == NULL) {
+VIR_FREE(nodeset);
+virStorageReportError(conn, VIR_ERR_XML_ERROR,
+%s, _(missing storage pool source device
path));
+goto cleanup;
+}
+ ret-source.initiator[i].iqnname = (char *)name;
+ }
+}
if (options-flags VIR_STORAGE_POOL_SOURCE_DEVICE) {
xmlNodePtr *nodeset = NULL;
int nsource, i;
diff --git a/src/storage_conf.h b/src/storage_conf.h
index 421d305..c2504be 100644
--- a/src/storage_conf.h
+++ b/src/storage_conf.h
@@ -202,7 +202,12 @@ struct _virStoragePoolSourceDevice {
} geometry;
};
-
+/* Initiator Attributes */
+typedef virStoragePoolSourceInitiatorAttr
*virStoragePoolSourceInitiatorAttrPtr;
+struct virStoragePoolSourceInitiatorAttr {
+char *iqnname;
+/* We could put other initiator specific things here */
+}
typedef struct _virStoragePoolSource virStoragePoolSource;
typedef virStoragePoolSource *virStoragePoolSourcePtr;
@@ -214,6 +219,9 @@ struct _virStoragePoolSource {
int ndevice;
virStoragePoolSourceDevicePtr devices;
+/*And either one or more initiators*/
+virStoragePoolSourceInitiatorAttrPtr initiator;
+
/* Or a directory */
char *dir;
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
[libvirt] Resubmission #2: [PATCH 0/3] sVirt AppArmor security driver
Resubmitting again based on feedback from this list. Notably, the driver has been reworked to use: xml = virDomainDefFormat(conn, vm-def, ... Then the helper program uses: ctl-def = virDomainDefParseString(... xmlStr ...); This simplifies adding new files. Part of this process involved adding support for kernel, initrd, loader, serial, and console. I have commented out code for hostdev (and pci would be easy enough), but it requires a patch to to move the _usbDevice struct fully into hostusb.h. I figure once the AppArmor driver is in, I can submit a patch for that. This iteration also properly works with attach/detach. While I use virDomainDefFormat() primarily, this does not work with attach because the definition is not updated with the information yet (neither def nor newDef), so I pass the disk file on the command line. For now this is ok since SetSecurityImageLabel currently only deals with a single file, but this will need to be adjusted depending on how the storage backend patch works out. I thought about creating a new def and inserting the disk into it and sending it off to virt-aa-helper, but I couldn't find an easy way to get an accurate virCapsPtr in security_apparmor.c. I had a similar problem with virt-aa-helper, but there I just created a fake virCapsPtr (which defaults to 'hvm', which was a compromise I was willing to make at this time since sVirt only supports qemu right now). The issues with virCapsPtr aside, the code is overall cleaner and much more easily extendable, so thanks for the excellent feedback. :) The patches are against up to date trunk, compiles with no warnings, 'make check' passes for the tests I added, and the code passes syntax-check. With the exception of the three calls to strncpy I had to convert to virStr*, this is the code that is currently in Ubuntu 9.10 (0.7.0-1ubuntu8). Among other things, that means 9.10 now has your preferred licensing. [PATCH 1] patch_1_reenable-nonfile-labels.patch (Updated based on prior feedback): When James Morris originally submitted his sVirt patches (as seen in libvirt 0.6.1), he did not require on disk labelling for virSecurityDomainRestoreImageLabel. A later commit[2] changed this behavior to assume on disk labelling, which halts implementations for path-based MAC systems such as AppArmor and TOMOYO where vm-def-seclabel is required to obtain the label. This patch simply adds the 'virDomainObjPtr vm' argument back to *RestoreImageLabel. [PATCH 2] patch_2_apparmor_driver.patch (updated and merged into one patch based on prior feedback): - Updates src/security.c for AppArmor - Adds security_apparmor.c, security_apparmor.h, virt-aa-helper.c and updates po/POTFILES.in. virt-aa-helper.c is a new binary which is used exclusively by the AppArmor security driver to manipulate AppArmor. - Adds tests for virt-aa-helper and the security driver. secaatest.c is identical to seclabeltest.c except it initializes the 'apparmor' driver instead of 'selinux'. These tests are integrated into 'make check' and pass. - Updates configure.in, Makefile.am, src/Makefile.am, tests/Makefile.am, and tools/Makefile.am for AppArmor. It is based on and should operate the same as the SELinux configuration. [PATCH 3] patch_3_docs.patch (Updated based on prior feedback): Updates docs/drvqemu.html.in for AppArmor and adds profile examples to examples/apparmor. Updated based on prior feedback. Jamie On Fri, 04 Sep 2009, Jamie Strandboge wrote: This patch series implements the AppArmor security driver for sVirt. This implementation was developed for the Ubuntu AppArmorLibvirtProfile specification[1], but is general enough for any AppArmor deployment (such as Ubuntu, *SUSE and Mandriva). This patch has seen quite a bit of real world testing in Ubuntu 9.10 (our development release) in our 0.7.0-1ubuntu3 package. I did make a few small changes after going through HACKING, but mostly I got the tests going and added documentation. DESIGN -- When a virtual machine is started, determine if a profile is currently defined for the machine, and use it if available. If not, generate a new profile for the machine based on a template, which is by default a very restrictive profile allowing access to disk files, and anything else needed to run, such as the pid, monitor and log files. Virtual machines should have a unique profile specific to that machine. To ensure uniqueness, the profile name will be derived from the UUID of the virtual machine. These profiles should be configurable, either by adjusting the profile template for new machines, creating/modifying the VM profile directly or through the use of AppArmor abstractions. This will allow for administrators to fine-tune confinement for individual machines if desired. If enabled at compile time, the sVirt security model will be activated if AppArmor is available on the host OS and a profile for the libvirtd daemon is loaded when libvirtd is started. libvirtd should
[libvirt] Resubmission #2: [PATCH 1/3] sVirt AppArmor security driver
On Fri, 25 Sep 2009, Jamie Strandboge wrote:
[PATCH 1]
patch_1_reenable-nonfile-labels.patch (Updated based on prior feedback):
When James Morris originally submitted his sVirt patches (as seen in
libvirt 0.6.1), he did not require on disk labelling for
virSecurityDomainRestoreImageLabel. A later commit[2] changed this
behavior to assume on disk labelling, which halts implementations for
path-based MAC systems such as AppArmor and TOMOYO where
vm-def-seclabel is required to obtain the label. This patch simply
adds the 'virDomainObjPtr vm' argument back to *RestoreImageLabel.
--
Jamie Strandboge | http://www.canonical.com
diff -Naurp libvirt.orig/src/qemu/qemu_driver.c libvirt/src/qemu/qemu_driver.c
--- libvirt.orig/src/qemu/qemu_driver.c 2009-09-25 10:50:21.0 -0500
+++ libvirt/src/qemu/qemu_driver.c 2009-09-25 16:56:32.0 -0500
@@ -6309,7 +6309,7 @@ static int qemudDomainDetachDevice(virDo
dev-data.disk-bus == VIR_DOMAIN_DISK_BUS_VIRTIO)) {
ret = qemudDomainDetachPciDiskDevice(dom-conn, vm, dev);
if (driver-securityDriver)
-driver-securityDriver-domainRestoreSecurityImageLabel(dom-conn, dev-data.disk);
+driver-securityDriver-domainRestoreSecurityImageLabel(dom-conn, vm, dev-data.disk);
if (qemuDomainSetDeviceOwnership(dom-conn, driver, dev, 1) 0)
VIR_WARN0(Fail to restore disk device ownership);
} else if (dev-type == VIR_DOMAIN_DEVICE_NET) {
diff -Naurp libvirt.orig/src/security/security_driver.h libvirt/src/security/security_driver.h
--- libvirt.orig/src/security/security_driver.h 2009-09-22 12:51:57.0 -0500
+++ libvirt/src/security/security_driver.h 2009-09-25 16:56:32.0 -0500
@@ -32,6 +32,7 @@ typedef virSecurityDriverStatus (*virSec
typedef int (*virSecurityDriverOpen) (virConnectPtr conn,
virSecurityDriverPtr drv);
typedef int (*virSecurityDomainRestoreImageLabel) (virConnectPtr conn,
+ virDomainObjPtr vm,
virDomainDiskDefPtr disk);
typedef int (*virSecurityDomainSetImageLabel) (virConnectPtr conn,
virDomainObjPtr vm,
diff -Naurp libvirt.orig/src/security/security_selinux.c libvirt/src/security/security_selinux.c
--- libvirt.orig/src/security/security_selinux.c 2009-09-22 12:51:57.0 -0500
+++ libvirt/src/security/security_selinux.c 2009-09-25 16:56:32.0 -0500
@@ -377,6 +377,7 @@ err:
static int
SELinuxRestoreSecurityImageLabel(virConnectPtr conn,
+ virDomainObjPtr vm ATTRIBUTE_UNUSED,
virDomainDiskDefPtr disk)
{
/* Don't restore labels on readoly/shared disks, because
@@ -581,7 +582,8 @@ SELinuxRestoreSecurityLabel(virConnectPt
rc = -1;
}
for (i = 0 ; i vm-def-ndisks ; i++) {
-if (SELinuxRestoreSecurityImageLabel(conn, vm-def-disks[i]) 0)
+if (SELinuxRestoreSecurityImageLabel(conn, vm,
+ vm-def-disks[i]) 0)
rc = -1;
}
VIR_FREE(secdef-model);
signature.asc
Description: Digital signature
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Resubmission #2: [PATCH 3/3] sVirt AppArmor security driver
On Fri, 25 Sep 2009, Jamie Strandboge wrote: [PATCH 3] patch_3_docs.patch (Updated based on prior feedback): Updates docs/drvqemu.html.in for AppArmor and adds profile examples to examples/apparmor. Updated based on prior feedback. -- Jamie Strandboge | http://www.canonical.com diff -Naurp libvirt.orig/docs/drvqemu.html.in libvirt/docs/drvqemu.html.in --- libvirt.orig/docs/drvqemu.html.in 2009-09-02 14:34:08.0 -0500 +++ libvirt/docs/drvqemu.html.in 2009-09-25 17:03:19.0 -0500 @@ -296,6 +296,73 @@ file can be used to change the setting to codesecurity_driver=none/code /p +h3a name=securitysvirtaaAppArmor sVirt confinement/a/h3 + +p + When using basic AppArmor protection for the libvirtd daemon and + QEMU virtual machines, the intention is to protect the host OS + from a compromised virtual machine process. There is no protection + between guests. +/p + +p + The AppArmor sVirt protection for QEMU virtual machines builds on + this basic level of protection, to also allow individual guests to + be protected from each other. +/p + +p + In the sVirt model, if a profile is loaded for the libvirtd daemon, + then each codeqemu:///system/code QEMU virtual machine will have + a profile created for it when the virtual machine is started if one + does not already exist. This generated profile uses a profile name + based on the UUID of the QEMU virtual machine and contains rules + allowing access to only the files it needs to run, such as its disks, + pid file and log files. Just before the QEMU virtual machine is + started, the libvirtd daemon will change into this unique profile, + preventing the QEMU process from accessing any file resources that + are present in another QEMU process or the host machine. +/p + +p + The AppArmor sVirt implementation is flexible in that it allows an + administrator to customize the template file in + code/etc/apparmor.d/libvirt/TEMPLATE/code for site-specific + access for all newly created QEMU virtual machines. Also, when a new + profile is generated, two files are created: + code/etc/apparmor.d/libvirt/libvirt-lt;uuidgt;/code and + code/etc/apparmor.d/libvirt/libvirt-lt;uuidgt;.files/code. The + former can be fine-tuned by the administrator to allow custom access + for this particular QEMU virtual machine, and the latter will be + updated appropriately when required file access changes, such as when + a disk is added. This flexibility allows for situations such as + having one virtual machine in complain mode with all others in + enforce mode. +/p + +p + While users can define their own AppArmor profile scheme, a typical + configuration will include a profile for code/usr/sbin/libvirtd/code, + code/usr/bin/virt-aa-helper/code (a helper program which the + libvirtd daemon uses instead of manipulating AppArmor directly), and + an abstraction to be included by code/etc/apparmor.d/libvirt/TEMPLATE/code + (typically code/etc/apparmor.d/abstractions/libvirt-qemu/code). + An example profile scheme can be found in the examples/apparmor + directory of the source distribution. +/p + +p + If the sVirt security model is active, then the node capabilities + XML will include its details. If a virtual machine is currently + protected by the security model, then the guest XML will include + its assigned profile name. If enabled at compile time, the sVirt + security model will be activated if AppArmor is available on the host + OS and a profile for the libvirtd daemon is loaded when libvirtd is + started. To disable sVirt, and revert to the basic level of AppArmor + protection (host protection only), the code/etc/libvirt/qemu.conf/code + file can be used to change the setting to codesecurity_driver=none/code. +/p + h3a name=securityaclCgroups device ACLs/a/h3 diff -Naurp libvirt.orig/examples/apparmor/libvirt-qemu libvirt/examples/apparmor/libvirt-qemu --- libvirt.orig/examples/apparmor/libvirt-qemu 1969-12-31 18:00:00.0 -0600 +++ libvirt/examples/apparmor/libvirt-qemu 2009-09-25 17:03:19.0 -0500 @@ -0,0 +1,71 @@ +# Last Modified: Wed Jul 8 09:57:41 2009 + + #include abstractions/base + #include abstractions/consoles + #include abstractions/nameservice + + # required for reading disk images + capability dac_override, + capability dac_read_search, + capability chown, + + network inet stream, + network inet6 stream, + + /dev/net/tun rw, + /dev/kvm rw, + /dev/ptmx rw, + /dev/kqemu rw, + + # WARNING: uncommenting these gives the guest direct access to host hardware. + # This is required for USB pass through but is a security risk. You have been + # warned. + #/sys/bus/usb/devices/ r, + #/sys/devices/*/*/usb[0-9]*/** r, +
[libvirt] Fix memleaks in libvirtd's message processing
I tracked down a memleak in libvirtd's message processing. The leak
was introduced in commit 47cab734995fa9521b1df05d37e9978eedd8d3a2
Split out code for handling incoming method call messages. This
commit changed the way qemud_client_message objects were reused.
Before this commit:
- qemudWorker() removes a message from dispatch queue and passes it to
remoteDispatchClientRequest()
- remoteDispatchClientRequest() handles the message and reuses the
same message for the response. If an error occurs the same message is
used to report it (rpc_error label). If another error occurs while
trying to report the first error remoteDispatchClientRequest() would
return -1 and the message will be freed in qemudWorker()
After this commit:
- qemudWorker() removes a message from dispatch queue and passes it to
remoteDispatchClientRequest()
- remoteDispatchClientRequest() calls remoteDispatchClientCall() if
the message it is an remote call or otherwise respond with a new error
message by calling remoteSerializeReplyError() and the original
message leaks
- remoteDispatchClientCall() handles the message and reuses the same
message for the response. If an error occurs it calls
remoteSerializeReplyError() and the original message leaks. If a fatal
error occurs remoteDispatchClientCall() returns -1 and the original
message will be freed in qemudWorker()
To fix this leak the original message has to be freed if
remoteSerializeReplyError() succeeds. If remoteSerializeReplyError()
fails the original message will be freed in qemudWorker().
In addition I came across another leak in remoteSerializeError(). If
an error occurs while serializing the initial error the message leaks.
To fix this leak the message has to be freed in case of an XDR error.
Matthias
diff --git a/daemon/dispatch.c b/daemon/dispatch.c
index a60f2f4..ddb3215 100644
--- a/daemon/dispatch.c
+++ b/daemon/dispatch.c
@@ -191,6 +191,7 @@ remoteSerializeError(struct qemud_client *client,
xdr_error:
xdr_destroy(xdr);
+VIR_FREE(msg);
fatal_error:
xdr_free((xdrproc_t)xdr_remote_error, (char *)rerr);
return -1;
@@ -336,6 +337,7 @@ remoteDispatchClientRequest (struct qemud_server *server,
struct qemud_client *client,
struct qemud_client_message *msg)
{
+int ret;
remote_error rerr;
memset(rerr, 0, sizeof rerr);
@@ -364,7 +366,12 @@ remoteDispatchClientRequest (struct qemud_server *server,
}
error:
-return remoteSerializeReplyError(client, rerr, msg-hdr);
+ret = remoteSerializeReplyError(client, rerr, msg-hdr);
+
+if (ret = 0)
+VIR_FREE(msg);
+
+return ret;
}
@@ -521,8 +528,12 @@ remoteDispatchClientCall (struct qemud_server *server,
rpc_error:
/* Semi-bad stuff happened, we can still try to send back
* an RPC error message to client */
-return remoteSerializeReplyError(client, rerr, msg-hdr);
+rv = remoteSerializeReplyError(client, rerr, msg-hdr);
+
+if (rv = 0)
+VIR_FREE(msg);
+return rv;
xdr_error:
/* Seriously bad stuff happened, so we'll kill off this client
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Race between monitor startup and incoming migration impacting libvirt
There appears to be a race condition wherein a 'cont' command sent immediately on qemu startup can prevent a inbound migration specified via -incoming from occurring. libvirt's process for starting up qemu domains with an incoming migration includes with a 'cont' command at the end of qemudInitCpus, shortly after a successful connection with the monitor is made. While the libvirt monitor is generally unresponsive while an inbound migration is ongoing, forcing the 'cont' to occur only after the migration has completed, this isn't always true (as will be demonstrated below). I suspect strongly that this is responsible for an occasional failure I'm seeing when loading libvirt domains from file. This is highly reproducible using qemu-kvm-0.11.0-rc2, and straightforward to demonstrate by the following means: [ONE-TIME SETUP] - Build an appropriate ramsave file via migrating a stopped guest to disk. - Mark any backing store used by this guest read-only. [COMMON STEPS] - Create an empty qcow2 file backed by the read-only store, if your guest has any disks. - Invoke qemu with arguments appropriate to the VM being resumed, and also the following: -S -monitor stdio -incoming 'exec:echo START_DELAY 2 sleep 5 echo END_DELAY 2 cat ramsave.raw echo LOAD_DONE 2'. [VALIDATING CORRECT OPERATION] - Wait until 'LOAD_DONE' is displayed, and run 'cont' - The VM will correctly resume. [REPRODUCING THE BUG] - Run 'cont' after START_DELAY is displayed, but before END_DELAY. - 'cat: write error: Broken pipe' will be displayed. - The guest VM will reboot, enter a catatonic state, or otherwise fail to load correctly. [REPRODUCING WITHOUT ARTIFICIAL DELAY] As the 'sleep 5' used in the above may be considered cheating, this issue may also be reproduced without any delay by removing the 'sleep', and terminating the shell command used to invoke qemu with $'cont\n' [REPRODUCING OVER A UNIX SOCKET] Included for completeness, as libvirt 0.7.x uses UNIX sockets here. Use -monitor unix:tmp/test.monitor during qemu invocation, and - Invoke the following in a separate window: socat - UNIX-LISTEN:/tmp/test.monitor $'cont\n' - Invoke qemu as above, but with -monitor unix:/tmp/test.monitor I have a work-in-progress patch which modifies libvirt to use -daemonize for startup; waiting for the guest to detach before attempting to interact with the monitor may avoid this issue. However, as this patch is against libvirt master, and the master branch has other issues which expose themselves on virDomainRestore, I am unable to test it here. Thoughts (and workarounds) welcome. -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
RE: [libvirt] [RFC] Multi-IQN proposal
Thanks for the review. -Original Message- From: Dave Allan [mailto:dal...@redhat.com] Sent: Saturday, September 26, 2009 2:43 AM To: Iyer, Shyam Cc: libvir-list@redhat.com; Bellad, Sudhir; Domsch, Matt; KM, Paniraja Subject: Re: [libvirt] [RFC] Multi-IQN proposal shyam_i...@dell.com wrote: Would this proposal be acceptable ? In principle, I think what you're proposing is reasonable, and is certainly contemplated by the iSCSI specs. Example XML schema for an iSCSI storage pool created -- pool type=iscsi namedell/name uuidcf354733-b01b-8870-2040-94888640e24d/uuid capacity0/capacity allocation0/allocation available0/available - source initiator iqnname = initiator IQN1 initiator iqnname = initiator IQN2 host name=iSCSI target hostname or Target IP address / device path=iSCSI Target IQN name / /source - target path/dev/disk/by-path/path - permissions mode0700/mode owner0/owner group0/group /permissions /target /pool I think you have the initiator name specified in the right place in the XML. I would make the initiator iqn an element rather than an attribute, since your proposal contemplates adding additional initiator specific information later, and stylistically I think elements will be cleaner. That gives: initiator iqniqn.foo1.bar.baz/iqn iqniqn.foo2.bar.baz/iqn iqniqn.foo3.bar.baz/iqn /initiator Each initiator iqn name can be parsed to create the unique sessions. Fair enough. You should propose specifically how you see the sessions being set up. Each pool currently describes something that basically resembles a session, so your proposal modifies that paradigm a bit. Another possible way to implement what you describe would be to allow zero or one initiator tags within a pool. If no initiator tag is specified, the system will use the system default; if a tag is specified, the system will attempt to use the information contained in it. The more I think about it, the more I like that approach since it keeps the pool paradigm unmodified. Ok. This should solve the following possibilities -- * possibility of multiple IQNs for a single Guest * option for Guest's own BIOS initiator to use these IQNs (iSCSI in Guest) * option for hypervisor's initiator to use these IQNs on behalf of the guest (Multi-IQN) How is this different from the first possibility? The first possibility is usage 1 and 2(below) whereas the third possibility is usage 3 and 4(below) Compile tested only. Needs beatification. I didn't go over the code closely, but I didn't see anything that struck me as completely off base. I think it's more important to get the details of how this information will be used worked out at this point than to get the code finalized. Dave Example Usages: Usage 1: VM1 - Init iqn1 Target iqn1 Init iqn2 Target iqn1 Init iqn3 Target iqn1 Init iqn4 Target iqn1 Usage 2: VM1 - Init iqn1 Target iqn1 Init iqn2 Target iqn2 Init iqn3 Target iqn3 Init iqn4 Target iqn4 Usage 3: VM1 - Init iqn1 Target iqn1 VM2 - Init iqn2 Target iqn1 Usage 4: VM1 - Init iqn1 Target iqn1 VM2 - Init iqn2 Target iqn2 -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
