Hi,
thank you for your review
On Wed, Jan 16, 2013 at 9:29 PM, Marc-André Lureau
<marcandre.lureau@xxxxxxxxx> wrote:
> Hi
>
> On Wed, Dec 5, 2012 at 5:27 AM, Dunrong Huang <riegamaths@xxxxxxxxx> wrote:
>> The patch makes linux agent support file-xfer feature.
>>
>> Signed-off-by: Dunrong Huang <riegamaths@xxxxxxxxx>
>> ---
>> 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.
>
Sound great, I will try it.
>> 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.
>
Ok.
>> +/* 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.
>
will be fixed in next version
>> + }
>> + task = g_malloc0(sizeof(*task));
>
> nitpick: the more elegant g_new0(AgentFileXferTask, 1); is generally prefered.
>
ok
>> + 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.
>
will be fixed in next version.
>> +
>> + 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()
>
Agree, the code here was a bit ugly, will be fixed in next version.
>> + 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.
>
Ok.
>> + 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.
>
you are right, will be fixed in next version.
>> + 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? :)
>
you are right, there is no special purpose, write() should be enough.
>> + 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.
>
Ok.
>> + 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
>>
>> _______________________________________________
--
Best Regards,
Dunrong Huang
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel