[Spice-devel] [PATCH spice-vdagent] file-xfer: Check free space before file transfer

[Victor Toso]

Hi,

On Wed, May 03, 2017 at 04:27:30PM +0200, Jakub Janků wrote:
> Add function get_free_space_available that retrieves amount of free
> space in the given directory. The statvfs may fail even when there's
> enough free space (e.g. when not supported by system), in this case
> return G_MAXUINT64 so that the transfer isn't terminated groundlessly.
>
> When the file is too big, send VDAgentFileXferStatusMessage with
> result VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE and amount of free
> space available, if the result isn't supported by client, send
> VD_AGENT_FILE_XFER_STATUS_ERROR. Client then terminates the transfer.
> ---
>  src/vdagent/file-xfers.c | 31 +++++++++++++++++++++++++++++++
>  src/vdagentd/vdagentd.c  | 34 +++++++++++++++++++++++++++-------
>  2 files changed, 58 insertions(+), 7 deletions(-)
> 
> diff --git a/src/vdagent/file-xfers.c b/src/vdagent/file-xfers.c
> index b3937a4..49ccb87 100644
> --- a/src/vdagent/file-xfers.c
> +++ b/src/vdagent/file-xfers.c
> @@ -32,6 +32,7 @@
>  #include <fcntl.h>
>  #include <errno.h>
>  #include <sys/stat.h>
> +#include <sys/statvfs.h>
>  #include <sys/types.h>
>  #include <spice/vd_agent.h>
>  #include <glib.h>
> @@ -168,6 +169,18 @@ error:
>      return NULL;
>  }
>  
> +static uint64_t get_free_space_available(const char *path)
> +{
> +    struct statvfs stat;
> +    int error = statvfs(path, &stat);
> +    if (error != 0) {
> +        syslog(LOG_WARNING, "file-xfer: failed to get free space, statvfs error %d", error);

strerror(errno) instead?

> +        return G_MAXUINT64;
> +    } else {

You don't need the else here

> +        return stat.f_bsize * stat.f_bavail;

Quick test just to be sure, this is in bytes. Client also sends
file-size in bytes.

> +    }
> +}
> +
>  void vdagent_file_xfers_start(struct vdagent_file_xfers *xfers,
>      VDAgentFileXferStartMessage *msg)
>  {
> @@ -175,6 +188,7 @@ void vdagent_file_xfers_start(struct vdagent_file_xfers *xfers,
>      char *dir = NULL, *path = NULL, *file_path = NULL;
>      struct stat st;
>      int i;
> +    uint64_t free_space;
>  
>      g_return_if_fail(xfers != NULL);
>  
> @@ -193,6 +207,23 @@ void vdagent_file_xfers_start(struct vdagent_file_xfers *xfers,
>  
>      file_path = g_build_filename(xfers->save_dir, task->file_name, NULL);
>  
> +    free_space = get_free_space_available(xfers->save_dir);
> +    if (task->file_size > free_space) {
> +        syslog(LOG_ERR, "file-xfer: not enough free space (%lu B to copy, %lu B free)",
> +               task->file_size, free_space);
> +
> +        udscs_write(xfers->vdagentd,
> +                    VDAGENTD_FILE_XFER_STATUS,
> +                    msg->id,
> +                    VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE,
> +                    (uint8_t *)&free_space,
> +                    sizeof(free_space));
> +        vdagent_file_xfer_task_free(task);
> +        g_free(file_path);
> +        g_free(dir);
> +        return;
> +    }
> +
>      dir = g_path_get_dirname(file_path);
>      if (g_mkdir_with_parents(dir, S_IRWXU) == -1) {
>          syslog(LOG_ERR, "file-xfer: Failed to create dir %s", dir);
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index f3ac606..c2da249 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -903,17 +903,37 @@ static void agent_read_complete(struct udscs_connection **connp,
>          }
>          break;
>      case VDAGENTD_FILE_XFER_STATUS:{
> -        VDAgentFileXferStatusMessage status;
> -        status.id = GUINT32_TO_LE(header->arg1);
> -        status.result = GUINT32_TO_LE(header->arg2);
> +        VDAgentFileXferStatusMessage *status;
> +        int status_size = sizeof(*status);
> +        uint32_t result = GUINT32_TO_LE(header->arg2);
> +
> +        if (result == VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE &&
> +            !VD_AGENT_HAS_CAPABILITY(capabilities, capabilities_size,
> +                                     VD_AGENT_CAP_FILE_XFER_FREE_SPACE)) {
> +            result = VD_AGENT_FILE_XFER_STATUS_ERROR;
> +        }
> +
> +        if (result == VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE) {
> +            status_size += sizeof(uint64_t);
> +            status = malloc(status_size);
> +            status->data[0] = *((uint64_t *)data);
> +        } else {
> +            status = malloc(status_size);
> +        }
> +
> +        status->result = result;
> +        status->id = GUINT32_TO_LE(header->arg1);
>          vdagent_virtio_port_write(virtio_port, VDP_CLIENT_PORT,
>                                    VD_AGENT_FILE_XFER_STATUS, 0,
> -                                  (uint8_t *)&status, sizeof(status));
> -        if (status.result == VD_AGENT_FILE_XFER_STATUS_CAN_SEND_DATA)
> -            g_hash_table_insert(active_xfers, GUINT_TO_POINTER(status.id),
> +                                  (uint8_t *)status, status_size);
> +
> +        if (status->result == VD_AGENT_FILE_XFER_STATUS_CAN_SEND_DATA)
> +            g_hash_table_insert(active_xfers, GUINT_TO_POINTER(status->id),
>                                  *connp);
>          else
> -            g_hash_table_remove(active_xfers, GUINT_TO_POINTER(status.id));
> +            g_hash_table_remove(active_xfers, GUINT_TO_POINTER(status->id));
> +
> +        free(status);

As it would be nice to report more error messages to the client (for
instance, failed due locked/login screen), it would be nice to have a
function that takes the error message as argument. Something around
send_file_xfer_status() perhaps?

Cheers,
        toso

>          break;
>      }
>  
> -- 
> 2.12.2
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170504/630791f8/attachment.sig>