Opened 13 years ago

Closed 12 years ago

#1982 closed Enhancement (wontfix)

RPC server ignores unknown arguments

Reported by: KyleK Owned by: charles
Priority: Normal Milestone: None Set
Component: libtransmission Version: 1.51+
Severity: Normal Keywords:
Cc:

Description

This is kind of an enhancement of ticket #1981.

I only noticed the issue because I wanted to test the new patch for #1894, and while Transmission didn't crash, it also didn't stop the torrents when the ratio was reached. Checking the source code I noticed the discrepancy in argument names in remote.c and rpcimpl.c (and docs/rpc-spec.txt).

But, what actually bothers me now is that transmission-remote (or rather the daemon) always responded with "success" when sending commands with wrong/unknown arguments.

Maybe the response should be more like "unkown argument" if the server doesn't understand the message?

Attachments (1)

warn_unknown_args_v1.patch (18.6 KB) - added by KyleK 13 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 13 years ago by charles

Could you make a patch for this?

comment:2 Changed 13 years ago by KyleK

I'll give it a try, although looking through the code it looks like this is a bigger change than I anticipated.

All the functions in rpcimpl.c search for specific arguments and only act if they find them. Unknown keywords are literally walked over :)

comment:3 Changed 13 years ago by KyleK

Ok, here's v1 of a patch.

It produces a warning if there are unknown arguments in an RPC command. There's currently no way to determine what is the unknown argument, though.

Also, if the command contained at least the required arguments, it will be executed regardless of whether there were unknown keywords (i.e. there's no rollback).

Changed 13 years ago by KyleK

comment:4 Changed 13 years ago by charles

KyleK: that's a pretty big diff for this feature. I hadn't realized it would be so large, and am kind of wondering if it's worth it to generate that warning.

comment:5 Changed 13 years ago by KyleK

I agree that the diff is rather large (and not very pretty). But with the extensive RPC interfac change in 1.60 (especially the renames) I think it is important to provide proper feedback to the user (or to the GUI interfaces out there). If you issue a command and the server responds "success" even though it didn't do anything (because none of the arguments were valid), seems pretty severe to me.

Maybe instead of checking each argument it'd be better to count the actions the server performs and then compare it with the original number of arguments.

Or the server response could include a "success" (or "fail") for each action.

If you have any other ideas how to make this more efficient I'll gladly do a rewrite.

PS: I'll be on vacation for the next 2 weeks, so it'd probably be best to put this on the back burner :)

comment:6 Changed 12 years ago by charles

KyleK: Any ideas on what to do next with this ticket?

comment:7 Changed 12 years ago by KyleK

I don't have to work for the next 4 days, I think I should find some time to revisit this issue. Maybe I can come up with a cleaner/smaller patch for this.

comment:8 Changed 12 years ago by charles

  • Resolution set to wontfix
  • Status changed from new to closed

This feature's code size / usefulness ratio has kept me somewhere between "no" and "maybe" on this ticket, and it doesn't seem to be going anywhere, so I'm going to close it for now.

If KyleK, or anyone else, wants to work up a simple/clean patch for this I'd be happy to review it.

KyleK: I'm a little worried that you might take umbrage that I'm closing two of your tickets in one day... it's nothing personal, so don't read anything into it. :)

Note: See TracTickets for help on using tickets.