Handle ERROR 405 (Method Not Allowed)

[lars18th]

Hi,

In the function "bool cSatipRtsp::ValidateLatestResponse(long *rcP)"
http://github.com/rofafor/vdr-plugin-satip/blob/4e9b6f11eb606398f366e0c9502b6e3f8db07a22/rtsp.c#L396
only responses 200 (OK) and 402/403/503 are processed.

However, several other relevant responses are not right catched!
For example, 405 (Method Not Allowed) is returned by the SAT>IP server when the current request can't be processed (now and after). This is true, for example, when the frequency is not supported. Some servers (like TVHE) use this behaviour as describes the standard.

So, please, can you add support for this response?

[rofafor]

I don't quite understand what you're looking after. The 405 status is catched in the default case and the special error cases are there just to provide extra information from extra header line they are providing.

If the frequency isn't supported, shouldn't TvH return 403 and put the frequency field into "Out-of-Range:" header? IMO, the 405 is reserved for other uses like invalid url construction etc.

[lars18th]

Hi,

Regarding TvH: When the freq. is not available, but valid, then it responds with 405 status, and not 403.

The specification of SAT>IP protocol says:

Status code 403 – Forbidden
Description The server understood the request, but is refusing to fulfil it. Returned when passing an attribute value not understood by the server in a query, or an out-of-range value.

So, here the problem is some parameter that the server don't likes!

Status code 405 – Method Not Allowed
Description The method specified in the request is not allowed for the resource identified by the Request-URI. Returned when applying a SETUP, PLAY or TEARDOWN method on an RTSP URI identifying the server.

And in this case, the server is indicating that the request is valid, but is not allowed to serve this request.

So, my suggestion is provide specific catch of all status responses listed in the standard, and not only 200, 402/403/503:

Status code Reason Phrase
100 Continue
200 OK
400 Bad Request
403 Forbidden
404 Not Found
405 Method Not Allowed
406 Not Acceptable
408 Request Timeout
414 Request-URI Too Long
453 Not Enough Bandwidth
454 Session Not Found
455 Method Not Valid in This State
461 Unsupported Transport
500 Internal Server Error
501 Not Implemented
503 Service Unavailable
505 RTSP Version Not Supported
551 Option Not Supported

The reason is improve compatibility with some more SAT>IP servers.

[rofafor]

I just don't get what you're looking after: how about providing a code example? I'm already catching all of those responses via a switch-case expression in cSatipRtsp::ValidateLatestResponse(). The 200 has a special handling as it shouldn't produce any error log messages or failed return values. The 400h as a special handling as the response might contain a Check-Syntax header for extra info about the root cause. The 403 has a special handling as the response might contain a Out-of-Range header for extra info about the root cause. The 503 has a special handling as the response might contain a No-More header for extra info about the root cause. Now, if the specs contains other special response headers for defined status codes, let me know and I'll add a support for them. But if you want magic how the plugin handles gracefully certain error situation, you'll have to provide a PR as they usually aren't simple to implement due to the VDR API interface that's been designed for kernel device interface only.

Off-topic: what does it exactly mean, that frequency is not available (but valid)?

[lars18th]

Hi @rofafor ,

You're right. I notice that it should be explained better.

what does it exactly mean, that frequency is not available (but valid)?

One example: One SAT>IP server connected to a fixed antena with only 1 range available (no diseqc or tone burst supported). For example, Astra 19.2 High Vertical. In this case, any request for tune Horizontal frequencies are a "valid" request, but this frequency can't be tuned. Then, the corresponding response from the server is 405 status (Method Not Allowed).

how about providing a code example?
I'm already catching all of those responses via a switch-case expression in cSatipRtsp::ValidateLatestResponse()

Yes, you're right again. In the function at rtsp.c#L396, you catch all responses:
http://github.com/rofafor/vdr-plugin-satip/blob/4e9b6f11eb606398f366e0c9502b6e3f8db07a22/rtsp.c#L405

Where is then the problem?
The problem is in the code that uses the "result" from calling to ValidateLatestResponse():
http://github.com/rofafor/vdr-plugin-satip/blob/4e9b6f11eb606398f366e0c9502b6e3f8db07a22/rtsp.c#L167
http://github.com/rofafor/vdr-plugin-satip/blob/4e9b6f11eb606398f366e0c9502b6e3f8db07a22/rtsp.c#L222

As you return the bool result, then the working process treats the "error" as a "fatal error", because only "ok" or "error" are posible.

For example, see the function "bool cSatipTuner::Connect(void)":
http://github.com/rofafor/vdr-plugin-satip/blob/4e9b6f11eb606398f366e0c9502b6e3f8db07a22/tuner.c#L206
at lines L222 and L228 you check the calls to rtspM.Options() and rtspM.Setup(). And if one of these fails, then you execute

     rtspM.Reset();
     streamIdM = -1;
     error("Connect failed [device %d]", deviceIdM);

Why? If, for example, you receive a response with 405, you don't need to mark the connection as failed. It's only that the request is not acceptable, but you can continue.

Then, I suggest to modify the responses from bool to int, and add a new state, like "not-acceptable". This will fix, for example, the break of the scanning when you request a frequency in a transponder not available. In this case the server don't responds with 200 (OK) but 405 (Method Not Allowed) and the scanning process must just continue.

I hope I have explained better now! ;)

[rofafor]

Now I understand what you're looking for, but I still think your server implementation is malfunctioning in your example case: sending a SETUP method with an unsupported/invalid frequency value should return 403 status code with an extra header "Out-of-Range: freq". In this case I could validate that one as normal operation as re-tuning wouldn't fix the problem due to invalid channel configuration.

However, the status code 405 means that the method (in your example SETUP - not the frequency!) specified in the request is not allowed and all the allowed methods are listed in "Allow:" header. In this case the client and server states are most likely out-of-sync and tearing down the connection is easiest way to re-sync the communication (IMO).

[lars18th]

Hi @rofafor ,

My server is Tvh. And it is using 405 status, and not 403:
http://github.com/tvheadend/tvheadend/blob/a5358028063a8908f4b75fddf441f26c75770f92/src/satip/rtsp.c#L506

Related to how to understand the specification... perhaps you are right, and the correct response needs to be: status 403 and include in it the value "Out-of-Range: freq". However, several SAT>IP servers use this way: it responds with 405 status when the response can't be processed but it's valid.

So, even if the server sends 403... the behaviour is the same in your code. See:
http://github.com/rofafor/vdr-plugin-satip/blob/4e9b6f11eb606398f366e0c9502b6e3f8db07a22/rtsp.c#L418

In this code, the "case 403:" doesn't return "result = true;" and the net result is the same as with "default:". So, my suggestion continues to be valid. Futhermore, I feel that you can incorporate support for this case, as you say:

In this case I could validate that one as normal operation as re-tuning wouldn't fix the problem due to invalid channel configuration.

So, please, can you do it? I hope so!

[rofafor]

Implemented in 9c91e01. Can we close the issue?

[lars18th]

Hi @rofafor ,

Thank you for the change, but for sure this doesn't fixes the problem with THe! It responds with 405 instead of 403. My suggestion, as a simple workaround is:

rtsp.c

               }
-       case 403:
+       case 403,405:
            // SETUP PLAY TEARDOWN

Without this, your plugin still fails with all SAT>IP servers that returns 405 as "invalid frequency".
Please... ;)

[lars18th]

Hi @rofafor ,

Or best, add this:

       case 403:
...
               }
+       case 405:
+            // SETUP PLAY
+            // The server indicates that the resource is permanently unavailable.
+            error("Service permanently unavailable (error code %ld: %s)  [device %d]", rc, url, tunerM.GetId());
+            // Workaround:
+            //   Reseting the connection wouldn't help anything due to invalid channel configuration, so let it be successful
+            result = true;
       case 503:

I feel this is more correct!

[rofafor]

You'd better get the server fixed to comply with the SAT>IP standards:

403 Forbidden
The server understood the request, but is refusing to fulfil it. Returned when passing an attribute value not understood by the server in a query, or an out-of-range value.

If that's somehow impossible, you should provide me a PR with TvH detection for all the hacks the server requires. I don't want to break standard-compliant servers while doing this.

I'm closing this issue as won't fix and instead of reopen, please, open a pull request instead.

[lars18th]

Hi @rofafor ,

Your opinion is acceptable. I'll try to fix TvH.
Futhermore, I think the last commit, returning true when is not "Out-of-Range" in the response is the right way.

Thank you!