[Spice-devel] [PATCH spice-html5 3/4] Implement methods for transfering files from client to guest

[Pavel Grunt]

> 
> > +    if (msg.type == SPICE_MSG_MAIN_AGENT_DATA)
> > +    {
> > +        var agent_data = new SpiceMsgcMainAgentData(0, msg.data);
> 
> This should be a separate type; SpiceMsgMainAgentData, not Msgc.
> 
> > +SpiceMainConn.prototype.file_xfer_start = function(file)
> > +{
> > +    var task_id, xfer_start, task;
> > +
> > +    if (typeof this.file_xfer_tasks === "undefined")
> > +        this.file_xfer_tasks = {};
> 
> Isn't that redundant to the initialization above?
> 
Yes, it is.
> > +
> > +    task_id = Object.keys(this.file_xfer_tasks).length;
> 
> Aren't you running a risk of reusing task ids and running into
> possible
> collisions with this approach?
>

The task id is removed on the guest side as well when the task is completed, so there should be no problem. I can add the task counter.

>
>  > +    if (!this.file_xfer_tasks[file_xfer_status.id]) {
> 
> This code base is mostly not K&R brace style; if you could follow
> that,
> I'd appreciate it.
> 
> 
> > +        case VD_AGENT_FILE_XFER_STATUS_CANCELLED:
> > +            xfer_error = "transfer is cancelled by spice agent";
> > +            break;
> 
> How common is this case?  If it happens with any frequency, we may
> want
> to plan for some better clean up.  (e.g. don't uselessly read +
> transmit
> a huge file after it's been canceled).  You file onload callback
> could
> check for the existence of the task and abort if it vanishes.
> 
It didn't happened during my tests, but the check should be there.


Thanks for the review, I will send v2.

Pavel