[Qemu-devel] Re: [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs
On 12/03/10 19:03, Michael Roth wrote:
This implements a simple state machine to manage client/server rpc
jobs being multiplexed over a single channel.
A client job consists of sending an rpc request, reading an
rpc response, then making the appropriate callbacks. We allow one
client job to be processed at a time, which will make the following
state transitions:
VA_CLIENT_IDLE - VA_CLIENT_SEND (job queued, send channel open)
VA_CLIENT_SEND - VA_CLIENT_WAIT (request sent, awaiting response)
VA_CLIENT_WAIT - VA_CLIENT_IDLE (response recieved, callbacks made)
A server job consists of recieving an rpc request, generating a
response, then sending the response. We expect to receive one server
request at a time due to the 1 at a time restriction for client jobs.
Server jobs make the following transitions:
VA_SERVER_IDLE - VA_SERVER_WAIT (recieved/executed request, send
channel busy, response deferred)
VA_SERVER_IDLE - VA_SERVER_SEND (recieved/executed request, send
channel open, sending response)
VA_SERVER_WAIT - VA_SERVER_SEND (send channel now open, sending
response)
VA_SERVER_SEND - VA_SERVER_IDLE (response sent)
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
As mentioned before, I really don't understand why this is part of QEMU,
the guest agent really should be able to run totally outside of QEMU.
+
+#define DEBUG_VA
+
+#ifdef DEBUG_VA
+#define TRACE(msg, ...) do { \
+fprintf(stderr, %s:%s():L%d: msg \n, \
+__FILE__, __FUNCTION__, __LINE__, ## __VA_ARGS__); \
+} while(0)
+#else
+#define TRACE(msg, ...) \
+do { } while (0)
+#endif
+
+#define LOG(msg, ...) do { \
+fprintf(stderr, %s:%s(): msg \n, \
+__FILE__, __FUNCTION__, ## __VA_ARGS__); \
+} while(0)
This must be like the 217th copy of these functions, could you please
use some of the code that is already in the tree, and make it generic if
needed.
+
+#define VERSION 1.0
+#define EOL \r\n
+
+#define VA_HDR_LEN_MAX 4096 /* http header limit */
+#define VA_CONTENT_LEN_MAX 2*1024*1024 /* rpc/http send limit */
+#define VA_CLIENT_JOBS_MAX 5 /* max client rpcs we can queue */
+#define VA_SERVER_JOBS_MAX 1 /* max server rpcs we can queue */
As mentioned last time, please make this stuff configurable and not hard
coded.
Cheers,
Jes
Re: [Qemu-devel] Re: [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs
On 12/07/2010 07:38 AM, Jes Sorensen wrote:
On 12/03/10 19:03, Michael Roth wrote:
This implements a simple state machine to manage client/server rpc
jobs being multiplexed over a single channel.
A client job consists of sending an rpc request, reading an
rpc response, then making the appropriate callbacks. We allow one
client job to be processed at a time, which will make the following
state transitions:
VA_CLIENT_IDLE - VA_CLIENT_SEND (job queued, send channel open)
VA_CLIENT_SEND - VA_CLIENT_WAIT (request sent, awaiting response)
VA_CLIENT_WAIT - VA_CLIENT_IDLE (response recieved, callbacks made)
A server job consists of recieving an rpc request, generating a
response, then sending the response. We expect to receive one server
request at a time due to the 1 at a time restriction for client jobs.
Server jobs make the following transitions:
VA_SERVER_IDLE - VA_SERVER_WAIT (recieved/executed request, send
channel busy, response deferred)
VA_SERVER_IDLE - VA_SERVER_SEND (recieved/executed request, send
channel open, sending response)
VA_SERVER_WAIT - VA_SERVER_SEND (send channel now open, sending
response)
VA_SERVER_SEND - VA_SERVER_IDLE (response sent)
Signed-off-by: Michael Rothmdr...@linux.vnet.ibm.com
As mentioned before, I really don't understand why this is part of QEMU,
the guest agent really should be able to run totally outside of QEMU.
+
+#define DEBUG_VA
+
+#ifdef DEBUG_VA
+#define TRACE(msg, ...) do { \
+fprintf(stderr, %s:%s():L%d: msg \n, \
+__FILE__, __FUNCTION__, __LINE__, ## __VA_ARGS__); \
+} while(0)
+#else
+#define TRACE(msg, ...) \
+do { } while (0)
+#endif
+
+#define LOG(msg, ...) do { \
+fprintf(stderr, %s:%s(): msg \n, \
+__FILE__, __FUNCTION__, ## __VA_ARGS__); \
+} while(0)
This must be like the 217th copy of these functions, could you please
use some of the code that is already in the tree, and make it generic if
needed.
+
+#define VERSION 1.0
+#define EOL \r\n
+
+#define VA_HDR_LEN_MAX 4096 /* http header limit */
+#define VA_CONTENT_LEN_MAX 2*1024*1024 /* rpc/http send limit */
+#define VA_CLIENT_JOBS_MAX 5 /* max client rpcs we can queue */
+#define VA_SERVER_JOBS_MAX 1 /* max server rpcs we can queue */
As mentioned last time, please make this stuff configurable and not hard
coded.
Yup, definitely on the TODO. Should be in the next round.
Cheers,
Jes
[Qemu-devel] Re: [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs
On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
+/* create new client job and then put it on the queue. this can be
+ * called externally from virtagent. Since there can only be one virtagent
+ * instance we access state via an object-scoped global rather than pass
+ * it around.
+ *
+ * if this is successful virtagent will handle cleanup of req_xml after
+ * making the appropriate callbacks, otherwise callee should handle it
+ */
Explain please. Do you mean caller should handle it? Are you trying to
say that this function, when successful, steals the reference to
req_xml?
+int va_client_job_add(xmlrpc_mem_block *req_xml, VAClientCallback *cb,
+ MonitorCompletion *mon_cb, void *mon_data)
+{
+int ret;
+VAClientJob *client_job;
+TRACE(called);
+
+client_job = va_client_job_new(req_xml, cb, mon_cb, mon_data);
+if (client_job == NULL) {
+return -EINVAL;
+}
+
+ret = va_push_client_job(client_job);
+if (ret != 0) {
+LOG(error adding client to queue: %s, strerror(ret));
+qemu_free(client_job);
+return ret;
+}
+
+return 0;
+}
--
Thanks,
Adam
[Qemu-devel] Re: [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs
On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
+/* create new client job and then put it on the queue. this can be
+ * called externally from virtagent. Since there can only be one virtagent
+ * instance we access state via an object-scoped global rather than pass
+ * it around.
+ *
+ * if this is successful virtagent will handle cleanup of req_xml after
+ * making the appropriate callbacks, otherwise callee should handle it
+ */
Explain please. Do you mean caller should handle it? Are you trying to
say that this function, when successful, steals the reference to
req_xml?
+int va_client_job_add(xmlrpc_mem_block *req_xml, VAClientCallback *cb,
+ MonitorCompletion *mon_cb, void *mon_data)
+{
+int ret;
+VAClientJob *client_job;
+TRACE(called);
+
+client_job = va_client_job_new(req_xml, cb, mon_cb, mon_data);
+if (client_job == NULL) {
+return -EINVAL;
+}
+
+ret = va_push_client_job(client_job);
+if (ret != 0) {
+LOG(error adding client to queue: %s, strerror(ret));
+qemu_free(client_job);
+return ret;
+}
+
+return 0;
+}
+
+/* create new server job and then put it on the queue in wait state
+ * this should only be called from within our read handler callback
+ */
Since this function is only 4 lines and has only one valid call site.
perhaps its better to fold it directly into the read handler callback.
+static int va_server_job_add(xmlrpc_mem_block *resp_xml)
+{
+VAServerJob *server_job;
+TRACE(called);
+
+server_job = va_server_job_new(resp_xml);
+assert(server_job != NULL);
+va_push_server_job(server_job);
+return 0;
+}
--
Thanks,
Adam
[Qemu-devel] Re: [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs
On 12/06/2010 03:54 PM, Adam Litke wrote:
On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
+/* create new client job and then put it on the queue. this can be
+ * called externally from virtagent. Since there can only be one virtagent
+ * instance we access state via an object-scoped global rather than pass
+ * it around.
+ *
+ * if this is successful virtagent will handle cleanup of req_xml after
+ * making the appropriate callbacks, otherwise callee should handle it
+ */
Explain please. Do you mean caller should handle it? Are you trying to
say that this function, when successful, steals the reference to
req_xml?
Yup, should be caller. And yes, cleanup duty gets taken over if the call
succeeds (after transmitting the request we have no need for the
req_xml, so it didn't seem to make sense to carry it around just so the
caller can free it when it gets it's call later on)
+int va_client_job_add(xmlrpc_mem_block *req_xml, VAClientCallback *cb,
+ MonitorCompletion *mon_cb, void *mon_data)
+{
+int ret;
+VAClientJob *client_job;
+TRACE(called);
+
+client_job = va_client_job_new(req_xml, cb, mon_cb, mon_data);
+if (client_job == NULL) {
+return -EINVAL;
+}
+
+ret = va_push_client_job(client_job);
+if (ret != 0) {
+LOG(error adding client to queue: %s, strerror(ret));
+qemu_free(client_job);
+return ret;
+}
+
+return 0;
+}
[Qemu-devel] Re: [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs
On 12/06/2010 03:57 PM, Adam Litke wrote:
On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
+/* create new client job and then put it on the queue. this can be
+ * called externally from virtagent. Since there can only be one virtagent
+ * instance we access state via an object-scoped global rather than pass
+ * it around.
+ *
+ * if this is successful virtagent will handle cleanup of req_xml after
+ * making the appropriate callbacks, otherwise callee should handle it
+ */
Explain please. Do you mean caller should handle it? Are you trying to
say that this function, when successful, steals the reference to
req_xml?
+int va_client_job_add(xmlrpc_mem_block *req_xml, VAClientCallback *cb,
+ MonitorCompletion *mon_cb, void *mon_data)
+{
+int ret;
+VAClientJob *client_job;
+TRACE(called);
+
+client_job = va_client_job_new(req_xml, cb, mon_cb, mon_data);
+if (client_job == NULL) {
+return -EINVAL;
+}
+
+ret = va_push_client_job(client_job);
+if (ret != 0) {
+LOG(error adding client to queue: %s, strerror(ret));
+qemu_free(client_job);
+return ret;
+}
+
+return 0;
+}
+
+/* create new server job and then put it on the queue in wait state
+ * this should only be called from within our read handler callback
+ */
Since this function is only 4 lines and has only one valid call site.
perhaps its better to fold it directly into the read handler callback.
+static int va_server_job_add(xmlrpc_mem_block *resp_xml)
+{
+VAServerJob *server_job;
+TRACE(called);
+
+server_job = va_server_job_new(resp_xml);
+assert(server_job != NULL);
+va_push_server_job(server_job);
+return 0;
+}
What I was mainly shooting for was to have the entry-points for adding
client and server jobs be clear and somewhat similar. I've actually
moved the read handler callback body into
virtagent-server.c:va_do_server_rpc() since then. So client jobs get
added by hmp/qmp-virtagent:va_do_rpc()-va_push_client_job() and server
jobs by read
handler-virtagent-server.c:va_do_server_rpc()-va_push_server_job().
