GNOME Bugzilla – Bug 723780
upnp: filter out devices on the local machine
Last modified: 2014-03-27 19:22:32 UTC
We already have sources for local media, and going through the TCP/IP subsystem for local files is not the best use of resources. Also, this avoids a usability issue: the user who enabled local media sharing, but does not know how rygel, grilo and upnp in general work might see "John Doe's Media" in Totem, see his files there and think it's the proper way to open them. The actual filtering is a bit hackish, because all we see is the URI provided by SSDP. There is a fallback to comparing hostnames, but because usually DNS is not configured in a home network, we should almost always see an IP address there. From that, we need to check if any interface is configured to use it. We could ask NetworkManager or connman, but that would grow an heavy and unwanted dependency; we could ask netlink, but that would fail outside of Linux; the simplest, although quite hackish, solution is to try and bind() to the remote address - if that succeeds, the address is local after all. The biggest downside to this solution, besides being Unix only due to EADDRNOTAVAIL, is that socket() can fail because of EMFILE/ENFILE, in which case we can't perform the check. Not a big deal maybe.
Created attachment 268305 [details] [review] upnp: filter out devices on the local machine
Really not interested in that. The media exported through rygel is not the same that's indexed in Tracker.
(In reply to comment #2) > Really not interested in that. The media exported through rygel is not the same > that's indexed in Tracker. It is most of time. Even if you don't configure rygel to export through Tracker (and I don't understand why you would do that...), stuff is most likely to be in Videos, and that's exported and indexed. And anyway, how do you plan to explain people what's the difference? They will go to Channels, see their stuff, and say "oh cool, here it is". Even if they know the difference, having the source right there, big clickable and good-looking is so better than going through the file picker or file manager that they will be tempted to use it anyway. I was, at least.
(In reply to comment #3) > (In reply to comment #2) > > Really not interested in that. The media exported through rygel is not the same > > that's indexed in Tracker. > > It is most of time. And sometimes it isn't. > Even if you don't configure rygel to export through Tracker Not using Tracker is the default for rygel's setup in the sharing panel. Because your local videos aren't necessarily the ones you want to share out. > (and I don't understand why you would do that...), stuff is most likely to be > in Videos, and that's exported and indexed. Again, assumptions that aren't true. > And anyway, how do you plan to explain people what's the difference? > They will go to Channels, see their stuff, and say "oh cool, here it is". Then we need to do better in the UI, like showing shared videos in a special way. > Even if they know the difference, having the source right there, big clickable > and good-looking is so better than going through the file picker or file > manager that they will be tempted to use it anyway. I was, at least. Just removing the local UPnP server isn't a good way to fix this. And it also breaks UPnP servers running as a different user, or as a system service. "Tagging" the UPnP source would still be useful to the front-ends, simply hiding it isn't.
I still believe local sources should be completely hidden from the UI, but if we wanted to that at a upper layer, how do I tag the source?
I think the best approach would be adding a configuration option to hide local servers. Something like "hide-local-servers". If application sets it to TRUE, then the plugin could hide those local servers.
(In reply to comment #6) > I think the best approach would be adding a configuration option to hide local > servers. Something like "hide-local-servers". If application sets it to TRUE, > then the plugin could hide those local servers. That wouldn't help front-ends special case it. (In reply to comment #5) > I still believe local sources should be completely hidden from the UI, but if > we wanted to that at a upper layer, how do I tag the source? There's no way to do that yet, but I can see a need for tagging sources with arbitrary strings, such as "radio", "tv", marking sources as interesting for certain locales/countries ("countrycode:it" for raitv), etc.
(In reply to comment #7) > (In reply to comment #6) > > I think the best approach would be adding a configuration option to hide local > > servers. Something like "hide-local-servers". If application sets it to TRUE, > > then the plugin could hide those local servers. > > That wouldn't help front-ends special case it. > Why not?
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > I think the best approach would be adding a configuration option to hide local > > > servers. Something like "hide-local-servers". If application sets it to TRUE, > > > then the plugin could hide those local servers. > > > > That wouldn't help front-ends special case it. > > > > Why not? Because it simply hides the source, instead of making sure that the front-end can do something special with it (like getting a full list of all the items to be merged in the "local" view).
(In reply to comment #9) > Because it simply hides the source, instead of making sure that the front-end > can do something special with it (like getting a full list of all the items to > be merged in the "local" view). Yes, which is what this issue is about. Hence the option. But if we want to generalize this issue, I like the proposal of tagging the sources. It was something I had considered some time ago. If Giovanni agrees with considering the generic issue, let's hold on the patch and go with the generic proposal.
(In reply to comment #10) > (In reply to comment #9) > > Because it simply hides the source, instead of making sure that the front-end > > can do something special with it (like getting a full list of all the items to > > be merged in the "local" view). > > Yes, which is what this issue is about. Hence the option. You can't show something in a different place if you can't get to it... > But if we want to generalize this issue, I like the proposal of tagging the > sources. It was something I had considered some time ago. > > If Giovanni agrees with considering the generic issue, let's hold on the patch > and go with the generic proposal.
(In reply to comment #11) > You can't show something in a different place if you can't get to it... Yes, I understand. But the original description is not about showing local servers in a different place, but just do not show them at all, as other sources (like Tracker) could be providing that information. In any case, as I said, I also prefer having a more generic approach. So just identifying the local sources so the application decides if must be shown in a different place or just hide them completely is my preference too.
Comment on attachment 268305 [details] [review] upnp: filter out devices on the local machine Let's mark this as needs-work to get it off the review list.
There's a simple "tags" property in bug 724019 which this patch could use. This patch also needs to make sure not to tag UPnP servers from other users but from the same machine.
(In reply to comment #14) > There's a simple "tags" property in bug 724019 which this patch could use. This > patch also needs to make sure not to tag UPnP servers from other users but from > the same machine. Giovanni, does "localhost" and "localuser" tags as documented in the patch from bug 724019 comment 9 suit you for this use case?
Created attachment 271027 [details] [review] upnp: tag sources on the local machine Recognize sources that correspond to rygel or similar software on the local machine, and add a "localhost" tag to the source. This will allow Totem or other applications to filter the source out. The actual recognition is a bit hackish, because all we see is the URI provided by SSDP. There is a fallback to comparing hostnames, but because usually DNS is not configured in a home network, we should almost always see an IP address there. From that, we need to check if any interface is configured to use it. We could ask NetworkManager or connman, but that would grow an heavy and unwanted dependency; we could ask netlink, but that would fail outside of Linux; the simplest, although quite hackish, solution is to try and bind() to the remote address - if that succeeds, the address is local after all. The biggest downside to this solution, besides being Unix only due to EADDRNOTAVAIL, is that socket() can fail because of EMFILE/ENFILE, in which case we can't perform the check. Not a big deal maybe.
Created attachment 271028 [details] [review] upnp: tag sources that belong to the same user If the source is found to be on the local machine, scan /proc/net/tcp to find the UID of the process that is listening on the socket, and tag the source if it's the same user as the one running the client.
Review of attachment 271027 [details] [review]: Can you split out is_localhost_url() into a separate file? We'll probably need to re-use it in the dleyna UPnP source at some point. ::: src/upnp/grl-upnp.c @@ +530,3 @@ + It's actually 64 on linux. + */ + char hostname_buffer[256]; Use constant for that, so you can reuse it in the gethostname call lower down.
Review of attachment 271028 [details] [review]: Ditto about splitting in another file. ::: src/upnp/grl-upnp.c @@ +545,3 @@ + uid_t uid; + + if (!g_file_get_contents ("/proc/net/tcp", &proc_net_tcp, NULL, NULL)) How big is that file going to get? I have 100k of data on my system without doing much at all. Reading line-by-line seems like a better idea to me.
Created attachment 271041 [details] [review] upnp: tag sources on the local machine Recognize sources that correspond to rygel or similar software on the local machine, and add a "localhost" tag to the source. This will allow Totem or other applications to filter the source out. The actual recognition is a bit hackish, because all we see is the URI provided by SSDP. There is a fallback to comparing hostnames, but because usually DNS is not configured in a home network, we should almost always see an IP address there. From that, we need to check if any interface is configured to use it. We could ask NetworkManager or connman, but that would grow an heavy and unwanted dependency; we could ask netlink, but that would fail outside of Linux; the simplest, although quite hackish, solution is to try and bind() to the remote address - if that succeeds, the address is local after all. The biggest downside to this solution, besides being Unix only due to EADDRNOTAVAIL, is that socket() can fail because of EMFILE/ENFILE, in which case we can't perform the check. Not a big deal maybe.
Created attachment 271042 [details] [review] upnp: tag sources that belong to the same user If the source is found to be on the local machine, scan /proc/net/tcp to find the UID of the process that is listening on the socket, and tag the source if it's the same user as the one running the client.
Review of attachment 271041 [details] [review]: Looks good to me.
Review of attachment 271042 [details] [review]: Looks good as well.
Ops, I forgot about this before hard code freeze kicked in. Would this be ok for .1, or should I wait until branched?
Sounds good for .1
Attachment 271041 [details] pushed as 2b4ab2b - upnp: tag sources on the local machine Attachment 271042 [details] pushed as 91c4ee1 - upnp: tag sources that belong to the same user