[Spice-devel] [PATCH vd_agent V3 4/4] file-xfer: Add file-xfer support for linux agent
Marc-André Lureau
marcandre.lureau at gmail.com
Wed Jan 16 05:29:31 PST 2013
Hi
On Wed, Dec 5, 2012 at 5:27 AM, Dunrong Huang <riegamaths at gmail.com> wrote:
> The patch makes linux agent support file-xfer feature.
>
> Signed-off-by: Dunrong Huang <riegamaths at gmail.com>
> ---
> src/vdagent.c | 171 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> src/vdagentd.c | 43 +++++++++++++++
> 2 files changed, 214 insertions(+)
>
> diff --git a/src/vdagent.c b/src/vdagent.c
> index 0af82b1..b6507ec 100644
> --- a/src/vdagent.c
> +++ b/src/vdagent.c
> @@ -34,12 +34,24 @@
> #include <sys/select.h>
> #include <sys/stat.h>
> #include <spice/vd_agent.h>
> +#include <glib.h>
>
> #include "udscs.h"
> #include "vdagentd-proto.h"
> #include "vdagentd-proto-strings.h"
> #include "vdagent-x11.h"
>
> +typedef struct AgentFileXferTask {
> + uint32_t id;
> + int file_fd;
> + uint64_t read_bytes;
> + char *file_name;
> + uint64_t file_size;
> + struct AgentFileXferTask *next;
> +} AgentFileXferTask;
> +
> +GList *agent_file_xfer_task_list = NULL;
Tbh, I wonder why not using a GHashTable instead, the code might get
even simpler.
> static const char *portdev = "/dev/virtio-ports/com.redhat.spice.0";
> static int debug = 0;
> static struct vdagent_x11 *x11 = NULL;
> @@ -47,6 +59,153 @@ static struct udscs_connection *client = NULL;
> static int quit = 0;
> static int version_mismatch = 0;
>
> +static gint file_xfer_task_find(gconstpointer a, gconstpointer b)
> +{
> + AgentFileXferTask *task = (AgentFileXferTask *)a;
> + uint32_t id = *(uint32_t *)b;
> +
> + if (task->id == id) {
> + return 0;
> + }
> +
> + return 1;
> +}
That would go away for example.
> +static const char *get_home_dir()
> +{
> + const char *homedir = g_getenv("HOME");
> + if (!homedir) {
> + homedir = g_get_home_dir();
> + }
> + return homedir;
> +}
Right, this is needed for glib < 2.36. Please add a comment, I might
not be the first one surprised.
> +/* Parse start messag then create a new file xfer task */
> +static AgentFileXferTask *vdagent_parse_start_msg(
> + VDAgentFileXferStartMessage *msg)
> +{
> + GKeyFile *keyfile = NULL;
> + AgentFileXferTask *task = NULL;
> +
> + keyfile = g_key_file_new();
> + if (g_key_file_load_from_data(keyfile,
> + (const gchar *)msg->data,
> + -1,
> + G_KEY_FILE_NONE, NULL) == FALSE) {
> + goto error;
Please add a GError and print it's content in error.
> + }
> + task = g_malloc0(sizeof(*task));
nitpick: the more elegant g_new0(AgentFileXferTask, 1); is generally prefered.
> + task->id = msg->id;
> + task->file_name = g_key_file_get_string(
> + keyfile, "vdagent-file-xfer", "name", NULL);
> + task->file_size = g_key_file_get_uint64(
> + keyfile, "vdagent-file-xfer", "size", NULL);
Please use a GError and go to error if it is set.
> +
> + g_key_file_free(keyfile);
> + return task;
> +
> +error:
> + if (task) {
> + g_free(task->file_name);
> + g_free(task);
> + }
It is nicer and more future proof to add a agent_file_xfer_task_free()
> + if (keyfile) {
> + g_key_file_free(keyfile);
> + }
> + return NULL;
> +}
> +
> +static void vdagent_file_xfer_start(VDAgentFileXferStartMessage *msg)
> +{
> + AgentFileXferTask *new;
> + char file_path[1024]; /* file path, limit to 1024 chars */
There is a define PATHMAX or something, but I don't think you need to
care about it here, you can just allocate the string.
> + new = vdagent_parse_start_msg(msg);
> + if (new == NULL) {
> + goto error;
> + }
> +
> + snprintf(file_path, sizeof(file_path), "%s/Desktop/%s",
> + get_home_dir(), new->file_name);
Please use g_build_filename () and g_get_user_special_dir
(G_USER_DIRECTORY_DESKTOP). You might not need the get_home_dir()
function anymore.
> + new->file_fd = open(file_path, O_CREAT | O_WRONLY, 0644);
> + if (new->file_fd == -1) {
> + syslog(LOG_ERR, "Create file error:%s\n", strerror(errno));
> + goto error;
> + }
> +
> + if (ftruncate(new->file_fd, new->file_size) < 0) {
> + close(new->file_fd);
> + goto error;
> + }
> +
> + agent_file_xfer_task_list = g_list_append(agent_file_xfer_task_list, new);
> +
> + udscs_write(client, VDAGENTD_FILE_XFER_STATUS,
> + msg->id, VD_AGENT_FILE_XFER_STATUS_CAN_SEND_DATA, NULL, 0);
> + return ;
> +
> +error:
> + udscs_write(client, VDAGENTD_FILE_XFER_STATUS,
> + msg->id, VD_AGENT_FILE_XFER_STATUS_ERROR, NULL, 0);
> + if (new) {
> + free(new->file_name);
> + free(new);
> + }
> +}
> +
> +static void vdagent_file_xfer_status(VDAgentFileXferStatusMessage *msg)
> +{
> + syslog(LOG_INFO, "task %d received response %d", msg->id, msg->result);
> +
> + if (msg->result == VD_AGENT_FILE_XFER_STATUS_CAN_SEND_DATA) {
> + /* Do nothing */
> + } else {
> + /* Error, remove this task */
> + GList *l;
> + l = g_list_find_custom(agent_file_xfer_task_list, &msg->id,
> + file_xfer_task_find);
> + if (l) {
> + AgentFileXferTask *task = l->data;
> + syslog(LOG_DEBUG, "user removed task %d, result: %d", msg->id, msg->result);
> + agent_file_xfer_task_list = g_list_remove(agent_file_xfer_task_list, task);
> + close(task->file_fd);
> + free(task->file_name);
> + free(task);
If you use a GHashTable, you should be able to call only
g_hash_table_remove (GINT_TO_POINTER(msg->id)), use
hash_table_new_full() with agent_file_xfer_task_free().
> +static void vdagent_file_xfer_data(VDAgentFileXferDataMessage *msg)
> +{
> + AgentFileXferTask *task;
> + int len;
> + GList *l;
> +
> + l = g_list_find_custom(agent_file_xfer_task_list, &msg->id,
> + file_xfer_task_find);
> + if (l == NULL) {
> + syslog(LOG_INFO, "Can not find task:%d", msg->id);
> + return ;
> + }
> +
> + task = l->data;
> + len = pwrite(task->file_fd, msg->data, msg->size, task->read_bytes);
> + if (len == -1) {
> + syslog(LOG_ERR, "write file error:%s\n", strerror(errno));
> + /* TODO: close, cancel dnd */
> + return ;
> + }
Why not just call write() since it's written sequentially? I rarely
use pwrite(), but perhaps it's better in some other way? :)
> + task->read_bytes += msg->size;
> + if (task->read_bytes >= task->file_size) {
It could be worthwhile to check the > condition and warn in this case
before finishing.
> + syslog(LOG_DEBUG, "task %d have been finished", task->id);
> + agent_file_xfer_task_list = g_list_remove(agent_file_xfer_task_list, task);
> + close(task->file_fd);
> + free(task->file_name);
> + free(task);
> + }
> +}
> +
> void daemon_read_complete(struct udscs_connection **connp,
> struct udscs_message_header *header, uint8_t *data)
> {
> @@ -82,6 +241,18 @@ void daemon_read_complete(struct udscs_connection **connp,
> version_mismatch = 1;
> }
> break;
> + case VDAGENTD_FILE_XFER_START:
> + vdagent_file_xfer_start((VDAgentFileXferStartMessage *)data);
> + free(data);
> + break;
> + case VDAGENTD_FILE_XFER_STATUS:
> + vdagent_file_xfer_status((VDAgentFileXferStatusMessage *)data);
> + free(data);
> + break;
> + case VDAGENTD_FILE_XFER_DATA:
> + vdagent_file_xfer_data((VDAgentFileXferDataMessage *)data);
> + free(data);
> + break;
> default:
> syslog(LOG_ERR, "Unknown message from vdagentd type: %d, ignoring",
> header->type);
> diff --git a/src/vdagentd.c b/src/vdagentd.c
> index bd5e0d4..7006d80 100644
> --- a/src/vdagentd.c
> +++ b/src/vdagentd.c
> @@ -215,6 +215,34 @@ static void do_client_clipboard(struct vdagent_virtio_port *vport,
> data, size);
> }
>
> +static void do_client_file_xfer(struct vdagent_virtio_port *vport,
> + VDAgentMessage *message_header,
> + uint8_t *data)
> +{
> + uint32_t msg_type;
> +
> + if (!active_session_conn) {
> + syslog(LOG_WARNING,
> + "Could not find an agent connnection belonging to the "
> + "active session, ignoring client clipboard request");
> + return;
> + }
> +
> + switch (message_header->type) {
> + case VD_AGENT_FILE_XFER_START:
> + msg_type = VDAGENTD_FILE_XFER_START;
> + break;
> + case VD_AGENT_FILE_XFER_STATUS:
> + msg_type = VDAGENTD_FILE_XFER_STATUS;
> + break;
> + case VD_AGENT_FILE_XFER_DATA:
> + msg_type = VDAGENTD_FILE_XFER_DATA;
> + break;
> + }
> +
> + udscs_write(active_session_conn, msg_type, 0, 0, data, message_header->size);
> +}
> +
> int virtio_port_read_complete(
> struct vdagent_virtio_port *vport,
> int port_nr,
> @@ -284,6 +312,11 @@ int virtio_port_read_complete(
> }
> do_client_clipboard(vport, message_header, data);
> break;
> + case VD_AGENT_FILE_XFER_START:
> + case VD_AGENT_FILE_XFER_STATUS:
> + case VD_AGENT_FILE_XFER_DATA:
> + do_client_file_xfer(vport, message_header, data);
> + break;
> default:
> syslog(LOG_WARNING, "unknown message type %d, ignoring",
> message_header->type);
> @@ -613,6 +646,16 @@ void agent_read_complete(struct udscs_connection **connp,
> return;
> }
> break;
> + case VDAGENTD_FILE_XFER_STATUS:{
> + VDAgentFileXferStatusMessage status;
> + status.id = header->arg1;
> + status.result = header->arg2;
> + vdagent_virtio_port_write(virtio_port, VDP_CLIENT_PORT,
> + VD_AGENT_FILE_XFER_STATUS, 0,
> + (uint8_t *)&status, sizeof(status));
> + break;
> + }
> +
> default:
> syslog(LOG_ERR, "unknown message from vdagent: %u, ignoring",
> header->type);
> --
> 1.8.0
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Thanks!
--
Marc-André Lureau
More information about the Spice-devel
mailing list