Re: [Qemu-devel] Re: [RFC][PATCH v5 04/21] virtagent: transport definitions and job callbacks
On 12/07/10 18:19, Michael Roth wrote:
On 12/07/2010 07:44 AM, Jes Sorensen wrote:
+static int va_end_of_header(char *buf, int end_pos)
+{
+return !strncmp(buf+(end_pos-2), \n\r\n, 3);
+}
Maybe I am missing something here, but it looks like you do a strncmp to
a char that is one past the end of the buffer, or? If this is
intentional, please document it.
buf+end_pos points to the last char we read (rather than being an offset
to the current position). So it stops comparing when it reaches
buf+end_pos (buf=0 + end_pos=2 implies 3 characters)
For some reason this confused the hell out of me when I looked over it
again as well. Alternatively I can do:
static int va_end_of_header(char *buf, int end_pos)
{
return !strncmp(buf+(end_pos-2), \n\r\n, 3);
}
...
va_end_of_header(s-hdr, s-hdr_pos - 1)
-
static int va_end_of_header(char *buf, int cur_pos)
{
return !strncmp(buf+(cur_pos-3), \n\r\n, 3);
}
...
va_end_of_header(s-hdr, s-hdr_pos);
It does seem easier to parse...
I would prefer this, somewhat easier to parse.
All this http parsing code leaves the question open why you do it
manually, instead of relying on a library?
Something like libcurl? At some point we didn't attempt to use libraries
provide by xmlrpc-c (which uses libcurl for http transport) for the
client and server. The problem there is that libcurl really wants and
tcp socket read and write from, whereas we need to support tcp/unix
sockets on the host side and isa/virtio serial ports on the guest side.
Even assuming we could hook in wrappers for these other types of
sockets/channels, there's also the added complexity since dropping
virtproxy of multiplexing HTTP/RPCs using a single stream, whereas
something like libcurl would, understandably, assume it has a dedicated
stream to read/write from. So we wouldn't really save any work or code,
unfortunately.
I guess I am just a little worried that we end up with errors in the
code that could have been solved by using a maintainer http library, but
if it isn't feasible I guess not.
Cheers,
Jes
[Qemu-devel] Re: [RFC][PATCH v5 04/21] virtagent: transport definitions and job callbacks
On 12/03/10 19:03, Michael Roth wrote:
+static void va_server_read_cb(const char *content, size_t content_len)
+{
+xmlrpc_mem_block *resp_xml;
+VAServerData *server_data = va_state-server_data;
+int ret;
+
+TRACE(called);
+resp_xml = xmlrpc_registry_process_call(server_data-env,
+server_data-registry,
+NULL, content, content_len);
+if (resp_xml == NULL) {
+LOG(error processing RPC request);
+goto out_bad;
+}
+
+ret = va_server_job_add(resp_xml);
+if (ret != 0) {
+LOG(error adding server job: %s, strerror(ret));
+}
+
+return;
+out_bad:
+/* TODO: should reset state here */
+return;
Looks like some missing error handling is needed here?
+static void va_rpc_parse_hdr(VAHTState *s)
+{
+int i, line_pos = 0;
+bool first_line = true;
+char line_buf[4096];
In 03/21 you defined VA_HDR_LEN_MAX to 4096, here you hard code the
value sounds like something begging to go wrong.
+static int va_end_of_header(char *buf, int end_pos)
+{
+return !strncmp(buf+(end_pos-2), \n\r\n, 3);
+}
Maybe I am missing something here, but it looks like you do a strncmp to
a char that is one past the end of the buffer, or? If this is
intentional, please document it.
All this http parsing code leaves the question open why you do it
manually, instead of relying on a library?
Cheers,
Jes
Re: [Qemu-devel] Re: [RFC][PATCH v5 04/21] virtagent: transport definitions and job callbacks
On 12/07/2010 07:44 AM, Jes Sorensen wrote:
On 12/03/10 19:03, Michael Roth wrote:
+static void va_server_read_cb(const char *content, size_t content_len)
+{
+xmlrpc_mem_block *resp_xml;
+VAServerData *server_data =va_state-server_data;
+int ret;
+
+TRACE(called);
+resp_xml = xmlrpc_registry_process_call(server_data-env,
+server_data-registry,
+NULL, content, content_len);
+if (resp_xml == NULL) {
+LOG(error processing RPC request);
+goto out_bad;
+}
+
+ret = va_server_job_add(resp_xml);
+if (ret != 0) {
+LOG(error adding server job: %s, strerror(ret));
+}
+
+return;
+out_bad:
+/* TODO: should reset state here */
+return;
Looks like some missing error handling is needed here?
+static void va_rpc_parse_hdr(VAHTState *s)
+{
+int i, line_pos = 0;
+bool first_line = true;
+char line_buf[4096];
In 03/21 you defined VA_HDR_LEN_MAX to 4096, here you hard code the
value sounds like something begging to go wrong.
+static int va_end_of_header(char *buf, int end_pos)
+{
+return !strncmp(buf+(end_pos-2), \n\r\n, 3);
+}
Maybe I am missing something here, but it looks like you do a strncmp to
a char that is one past the end of the buffer, or? If this is
intentional, please document it.
buf+end_pos points to the last char we read (rather than being an offset
to the current position). So it stops comparing when it reaches
buf+end_pos (buf=0 + end_pos=2 implies 3 characters)
For some reason this confused the hell out of me when I looked over it
again as well. Alternatively I can do:
static int va_end_of_header(char *buf, int end_pos)
{
return !strncmp(buf+(end_pos-2), \n\r\n, 3);
}
...
va_end_of_header(s-hdr, s-hdr_pos - 1)
-
static int va_end_of_header(char *buf, int cur_pos)
{
return !strncmp(buf+(cur_pos-3), \n\r\n, 3);
}
...
va_end_of_header(s-hdr, s-hdr_pos);
It does seem easier to parse...
All this http parsing code leaves the question open why you do it
manually, instead of relying on a library?
Something like libcurl? At some point we didn't attempt to use libraries
provide by xmlrpc-c (which uses libcurl for http transport) for the
client and server. The problem there is that libcurl really wants and
tcp socket read and write from, whereas we need to support tcp/unix
sockets on the host side and isa/virtio serial ports on the guest side.
Even assuming we could hook in wrappers for these other types of
sockets/channels, there's also the added complexity since dropping
virtproxy of multiplexing HTTP/RPCs using a single stream, whereas
something like libcurl would, understandably, assume it has a dedicated
stream to read/write from. So we wouldn't really save any work or code,
unfortunately.
Cheers,
Jes
[Qemu-devel] Re: [RFC][PATCH v5 04/21] virtagent: transport definitions and job callbacks
On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
+static void va_http_send_handler(void *opaque)
+{
+VAHTState *s = va_state-send_state;
+enum va_http_status http_status;
+int fd = va_state-fd;
+int ret;
+
+TRACE(called, fd: %d, fd);
+
+switch (s-state) {
Why is there a VA_SEND_START state when it always falls through to
VA_SEND_HDR? Is there any difference between these?
+case VA_SEND_START:
+s-state = VA_SEND_HDR;
+case VA_SEND_HDR:
+do {
+ret = write(fd, s-hdr + s-hdr_pos, s-hdr_len - s-hdr_pos);
+if (ret = 0) {
+break;
+}
+s-hdr_pos += ret;
+} while (s-hdr_pos s-hdr_len);
+if (ret == -1) {
+if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR) {
+return;
+} else {
+LOG(error writing header: %s, strerror(errno));
+goto out_bad;
+}
+} else if (ret == 0) {
+LOG(connected closed unexpectedly);
+goto out_bad;
+} else {
+s-state = VA_SEND_BODY;
+}
+case VA_SEND_BODY:
+do {
+ret = write(fd, s-content + s-content_pos,
+s-content_len - s-content_pos);
+if (ret = 0) {
+break;
+}
+s-content_pos += ret;
+} while (s-content_pos s-content_len);
+if (ret == -1) {
+if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR) {
+return;
+} else {
+LOG(error writing content: %s, strerror(errno));
+goto out_bad;
+}
+} else if (ret == 0) {
+LOG(connected closed unexpectedly);
+goto out_bad;
+} else {
+http_status = VA_HTTP_STATUS_OK;
+goto out;
+}
+default:
+LOG(unknown state);
+goto out_bad;
+}
+
+out_bad:
+http_status = VA_HTTP_STATUS_ERROR;
+out:
+s-send_cb(http_status, s-content, s-content_len);
+qemu_set_fd_handler(fd, va_http_read_handler, NULL, NULL);
+}
--
Thanks,
Adam
[Qemu-devel] Re: [RFC][PATCH v5 04/21] virtagent: transport definitions and job callbacks
On 12/06/2010 04:02 PM, Adam Litke wrote:
On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
+static void va_http_send_handler(void *opaque)
+{
+VAHTState *s =va_state-send_state;
+enum va_http_status http_status;
+int fd = va_state-fd;
+int ret;
+
+TRACE(called, fd: %d, fd);
+
+switch (s-state) {
Why is there a VA_SEND_START state when it always falls through to
VA_SEND_HDR? Is there any difference between these?
Nope, not at the moment. I'll stick with just _HDR for now, but if we
ever need to do some initialization or anything before we start
sending/reading that's what this would be for.
+case VA_SEND_START:
+s-state = VA_SEND_HDR;
+case VA_SEND_HDR:
+do {
+ret = write(fd, s-hdr + s-hdr_pos, s-hdr_len - s-hdr_pos);
+if (ret= 0) {
+break;
+}
+s-hdr_pos += ret;
+} while (s-hdr_pos s-hdr_len);
+if (ret == -1) {
+if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR) {
+return;
+} else {
+LOG(error writing header: %s, strerror(errno));
+goto out_bad;
+}
+} else if (ret == 0) {
+LOG(connected closed unexpectedly);
+goto out_bad;
+} else {
+s-state = VA_SEND_BODY;
+}
+case VA_SEND_BODY:
+do {
+ret = write(fd, s-content + s-content_pos,
+s-content_len - s-content_pos);
+if (ret= 0) {
+break;
+}
+s-content_pos += ret;
+} while (s-content_pos s-content_len);
+if (ret == -1) {
+if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR) {
+return;
+} else {
+LOG(error writing content: %s, strerror(errno));
+goto out_bad;
+}
+} else if (ret == 0) {
+LOG(connected closed unexpectedly);
+goto out_bad;
+} else {
+http_status = VA_HTTP_STATUS_OK;
+goto out;
+}
+default:
+LOG(unknown state);
+goto out_bad;
+}
+
+out_bad:
+http_status = VA_HTTP_STATUS_ERROR;
+out:
+s-send_cb(http_status, s-content, s-content_len);
+qemu_set_fd_handler(fd, va_http_read_handler, NULL, NULL);
+}
