Re: [systemd-devel] [RFC 3/6] proxy-discoveryd: Add PAC support through duktape js engine
On Mon, Apr 13, 2015 at 09:51:05AM -0700, Marcel Holtmann wrote: > Hi Zbyszek, > > >> --- a/Makefile.am > >> +++ b/Makefile.am > >> @@ -5895,12 +5895,19 @@ rootlibexec_PROGRAMS += \ > >> systemd-proxy-discoveryd > >> > >> systemd_proxy_discoveryd_SOURCES = \ > >> + src/proxy-discovery/duktape.h \ > >> + src/proxy-discovery/duktape.c \ > > These files are not included in the patch, how do you intend to ship > > them? > > I figured this could be included as a separate commit, when it will > be time to commit proxy-discoveryd someday. > Those 2 files represent more than 2 megabytes, so I did not want to > flood the ml. > >>> duktape should not be embedded in systemd. It's a separate project, > >>> which gets updates and has a life of its own. > >>> > >>> Preferably, duktape would be packaged by distributions and we would > >>> use it like any other external library. If that is not possible, we'll > >>> have to look for some other option, but only then. For example, > >>> git submodule is still better than embedding of a snapshot. > >> > >> that is not how this actually works with duktape. The release versions are > >> building a single duktape.[ch] that you can only find in the releases. > >> Otherwise you have to stitch that manually. It is pretty much designed to > >> be included into other project as a copy. That is why nobody has actually > >> packages this so far. > >> > >> So while I get that including copies of other libraries is generally bad > >> for security update reasons, but this one would qualify as an exception. > >> It is just that we have to monitor duktape upstream releases and with that > >> also update the duktape version in systemd. > >> > >> It is something that is not the greatest solution, but something that is > >> feasible. Keep in mind that embedding your JS engine right into your > >> application allows for optimizations that you really want. We have build > >> PACrunner against V8 and MozJS and these are two really painful > >> experiences. For anybody wanting to build a small system, dealing with > >> these two upstream packages is a nightmare. It sort of works in a large > >> distribution like Fedora, but anything small it is not an option. > > > > I know that embedding is the way that upstream suggests. But it is a > > bad fit for the way that systemd is distributed. Our primary consumers > > are distributions, and the first thing most distributions will do is > > to rip out the embedded copy in order to use a shared one. Something > > as big as a javascript engine is bound to have security updates and by > > embedding it systemd we would be forced to a) follow upstream changes, > > b) release systemd updates based on duktape releases. Not something that > > we want to do or would do very well. > > > > We really should try to base this on a javascript engine which > > provides a shared library. > > so do you know of a small Javascript engine with a proper license then? And > that also has no additional dependencies. No, but I don't think this can be the guiding principle here. We already have dependencies on a large number of things: gnutls, docbook, unifont, libmicrohttpd, python, and a bunch of smaller libraries. I know that another dependency might be difficult for embedded systems, but a "small" library might actually be more work to maintain the long run than bigger library which is easier to integrate. > I was not kidding when I said the V8 and MozJS are a nightmare when you want > to build a small system. So these two are pretty much out of the question for > the PAC service. I certainly can believe that ;) Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC 3/6] proxy-discoveryd: Add PAC support through duktape js engine
Hi Zbyszek, >> --- a/Makefile.am >> +++ b/Makefile.am >> @@ -5895,12 +5895,19 @@ rootlibexec_PROGRAMS += \ >> systemd-proxy-discoveryd >> >> systemd_proxy_discoveryd_SOURCES = \ >> + src/proxy-discovery/duktape.h \ >> + src/proxy-discovery/duktape.c \ > These files are not included in the patch, how do you intend to ship them? I figured this could be included as a separate commit, when it will be time to commit proxy-discoveryd someday. Those 2 files represent more than 2 megabytes, so I did not want to flood the ml. >>> duktape should not be embedded in systemd. It's a separate project, >>> which gets updates and has a life of its own. >>> >>> Preferably, duktape would be packaged by distributions and we would >>> use it like any other external library. If that is not possible, we'll >>> have to look for some other option, but only then. For example, >>> git submodule is still better than embedding of a snapshot. >> >> that is not how this actually works with duktape. The release versions are >> building a single duktape.[ch] that you can only find in the releases. >> Otherwise you have to stitch that manually. It is pretty much designed to be >> included into other project as a copy. That is why nobody has actually >> packages this so far. >> >> So while I get that including copies of other libraries is generally bad for >> security update reasons, but this one would qualify as an exception. It is >> just that we have to monitor duktape upstream releases and with that also >> update the duktape version in systemd. >> >> It is something that is not the greatest solution, but something that is >> feasible. Keep in mind that embedding your JS engine right into your >> application allows for optimizations that you really want. We have build >> PACrunner against V8 and MozJS and these are two really painful experiences. >> For anybody wanting to build a small system, dealing with these two upstream >> packages is a nightmare. It sort of works in a large distribution like >> Fedora, but anything small it is not an option. > > I know that embedding is the way that upstream suggests. But it is a > bad fit for the way that systemd is distributed. Our primary consumers > are distributions, and the first thing most distributions will do is > to rip out the embedded copy in order to use a shared one. Something > as big as a javascript engine is bound to have security updates and by > embedding it systemd we would be forced to a) follow upstream changes, > b) release systemd updates based on duktape releases. Not something that > we want to do or would do very well. > > We really should try to base this on a javascript engine which > provides a shared library. so do you know of a small Javascript engine with a proper license then? And that also has no additional dependencies. I was not kidding when I said the V8 and MozJS are a nightmare when you want to build a small system. So these two are pretty much out of the question for the PAC service. Regards Marcel ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC 3/6] proxy-discoveryd: Add PAC support through duktape js engine
On Mon, Apr 13, 2015 at 08:43:32AM -0700, Marcel Holtmann wrote: > Hi Zbyszek, > > --- a/Makefile.am > +++ b/Makefile.am > @@ -5895,12 +5895,19 @@ rootlibexec_PROGRAMS += \ > systemd-proxy-discoveryd > > systemd_proxy_discoveryd_SOURCES = \ > + src/proxy-discovery/duktape.h \ > + src/proxy-discovery/duktape.c \ > >>> These files are not included in the patch, how do you intend to ship them? > >> > >> I figured this could be included as a separate commit, when it will > >> be time to commit proxy-discoveryd someday. > >> Those 2 files represent more than 2 megabytes, so I did not want to > >> flood the ml. > > duktape should not be embedded in systemd. It's a separate project, > > which gets updates and has a life of its own. > > > > Preferably, duktape would be packaged by distributions and we would > > use it like any other external library. If that is not possible, we'll > > have to look for some other option, but only then. For example, > > git submodule is still better than embedding of a snapshot. > > that is not how this actually works with duktape. The release versions are > building a single duktape.[ch] that you can only find in the releases. > Otherwise you have to stitch that manually. It is pretty much designed to be > included into other project as a copy. That is why nobody has actually > packages this so far. > > So while I get that including copies of other libraries is generally bad for > security update reasons, but this one would qualify as an exception. It is > just that we have to monitor duktape upstream releases and with that also > update the duktape version in systemd. > > It is something that is not the greatest solution, but something that is > feasible. Keep in mind that embedding your JS engine right into your > application allows for optimizations that you really want. We have build > PACrunner against V8 and MozJS and these are two really painful experiences. > For anybody wanting to build a small system, dealing with these two upstream > packages is a nightmare. It sort of works in a large distribution like > Fedora, but anything small it is not an option. I know that embedding is the way that upstream suggests. But it is a bad fit for the way that systemd is distributed. Our primary consumers are distributions, and the first thing most distributions will do is to rip out the embedded copy in order to use a shared one. Something as big as a javascript engine is bound to have security updates and by embedding it systemd we would be forced to a) follow upstream changes, b) release systemd updates based on duktape releases. Not something that we want to do or would do very well. We really should try to base this on a javascript engine which provides a shared library. Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC 3/6] proxy-discoveryd: Add PAC support through duktape js engine
Hi Zbyszek, --- a/Makefile.am +++ b/Makefile.am @@ -5895,12 +5895,19 @@ rootlibexec_PROGRAMS += \ systemd-proxy-discoveryd systemd_proxy_discoveryd_SOURCES = \ + src/proxy-discovery/duktape.h \ + src/proxy-discovery/duktape.c \ >>> These files are not included in the patch, how do you intend to ship them? >> >> I figured this could be included as a separate commit, when it will >> be time to commit proxy-discoveryd someday. >> Those 2 files represent more than 2 megabytes, so I did not want to >> flood the ml. > duktape should not be embedded in systemd. It's a separate project, > which gets updates and has a life of its own. > > Preferably, duktape would be packaged by distributions and we would > use it like any other external library. If that is not possible, we'll > have to look for some other option, but only then. For example, > git submodule is still better than embedding of a snapshot. that is not how this actually works with duktape. The release versions are building a single duktape.[ch] that you can only find in the releases. Otherwise you have to stitch that manually. It is pretty much designed to be included into other project as a copy. That is why nobody has actually packages this so far. So while I get that including copies of other libraries is generally bad for security update reasons, but this one would qualify as an exception. It is just that we have to monitor duktape upstream releases and with that also update the duktape version in systemd. It is something that is not the greatest solution, but something that is feasible. Keep in mind that embedding your JS engine right into your application allows for optimizations that you really want. We have build PACrunner against V8 and MozJS and these are two really painful experiences. For anybody wanting to build a small system, dealing with these two upstream packages is a nightmare. It sort of works in a large distribution like Fedora, but anything small it is not an option. Regards Marcel ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC 3/6] proxy-discoveryd: Add PAC support through duktape js engine
On Mon, Apr 13, 2015 at 01:24:57PM +0300, Tomasz Bursztyka wrote: > Hi Tom, > > >>--- a/Makefile.am > >>+++ b/Makefile.am > >>@@ -5895,12 +5895,19 @@ rootlibexec_PROGRAMS += \ > >> systemd-proxy-discoveryd > >> > >> systemd_proxy_discoveryd_SOURCES = \ > >>+ src/proxy-discovery/duktape.h \ > >>+ src/proxy-discovery/duktape.c \ > >These files are not included in the patch, how do you intend to ship them? > > I figured this could be included as a separate commit, when it will > be time to commit proxy-discoveryd someday. > Those 2 files represent more than 2 megabytes, so I did not want to > flood the ml. duktape should not be embedded in systemd. It's a separate project, which gets updates and has a life of its own. Preferably, duktape would be packaged by distributions and we would use it like any other external library. If that is not possible, we'll have to look for some other option, but only then. For example, git submodule is still better than embedding of a snapshot. Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC 3/6] proxy-discoveryd: Add PAC support through duktape js engine
Le 10/04/2015 18:49, Lennart Poettering a écrit :
On Fri, 10.04.15 15:17, Tomasz Bursztyka (tomasz.burszt...@linux.intel.com)
wrote:
+struct PAC {
+duk_context *ctx;
+};
+
+static int get_addresses_from_interface(int ifindex, union in_addr_union
*address) {
+struct ifreq ifr = {};
+int sk;
+
+sk = socket(AF_INET, SOCK_DGRAM, 0);
+if (sk < 0)
+return -1;
No made up errors please! Return -errno or so.
+
+ifr.ifr_ifindex = ifindex;
+
+if (ioctl(sk, SIOCGIFNAME, &ifr) < 0 || ioctl(sk, SIOCGIFADDR, &ifr) <
0) {
+close(sk);
+return -1;
Same here...
Also, please don't use the old ioctls for querying network
information. Use netlink through sd-rtnl. You can look at the
systemd-resolved sources, they do this already, and this code should
probably do it very similar from that.
Yes, it was just to get something working as a PoC. But nothing to be
pushed.
I started to work on an sd-rtnl part that will keep track of the
interfaces, their IPs and
which one it tight to the default route.
+static int create_context(struct PAC *pac, char *pac_file, void *user_data) {
+duk_context *ctx;
+
+ctx = duk_create_heap(NULL, NULL, NULL, NULL, NULL);
+if (!ctx)
+return -ENOMEM;
+
+duk_push_global_object(ctx);
+duk_push_c_function(ctx, pac_dns_resolve, 1);
+duk_put_prop_string(ctx, -2, "dnsResolve");
+duk_push_c_function(ctx, pac_my_ip_address, 0);
+duk_put_prop_string(ctx, -2, "myIpAddress");
+
+duk_push_pointer(ctx, user_data);
+duk_put_prop_string(ctx, -2, "_user_data_");
+
+duk_pop(ctx);
+
+if (duk_peval_file(ctx, pac_file) != 0) {
+duk_destroy_heap(ctx);
+return -EINVAL;
+}
How is error handling done in duktape? The individual functions cannot
fail? And are any errors returned?
There are yes. Let's use them. I'll also hook the internal duktape
failure functions etc...
And I'll apply the other comments as well
Tomasz
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC 3/6] proxy-discoveryd: Add PAC support through duktape js engine
Hi Tom, --- a/Makefile.am +++ b/Makefile.am @@ -5895,12 +5895,19 @@ rootlibexec_PROGRAMS += \ systemd-proxy-discoveryd systemd_proxy_discoveryd_SOURCES = \ + src/proxy-discovery/duktape.h \ + src/proxy-discovery/duktape.c \ These files are not included in the patch, how do you intend to ship them? I figured this could be included as a separate commit, when it will be time to commit proxy-discoveryd someday. Those 2 files represent more than 2 megabytes, so I did not want to flood the ml. src/proxy-discovery/proxy-discoveryd.c \ src/proxy-discovery/proxy-discoveryd.h \ src/proxy-discovery/proxy-discoveryd-manager.c \ + src/proxy-discovery/proxy-discoveryd-pac.c \ src/proxy-discovery/proxy-discoveryd-proxy.c \ src/proxy-discovery/proxy-discoveryd-proxy-gperf.c +systemd_proxy_discoveryd_CFLAGS = \ + $(AM_CFLAGS) \ + -fno-fast-math Hm, what's this all about? duktape refuses to compile with fast-math directive. It can be forced however. I just avoided it for the PoC. +} + +address->in = ((struct sockaddr_in *) &ifr.ifr_addr)->sin_addr; + +close(sk); + +return 0; +} The above function is fine as part of a prototype, but for the final version we should use rtnl like everywhere else, no? Yep, I only did that for a working PoC. But rtnl based logic should be the way to go. I'll apply the return code comments. Tomasz ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC 3/6] proxy-discoveryd: Add PAC support through duktape js engine
On Fri, 10.04.15 15:26, Marcel Holtmann (mar...@holtmann.org) wrote:
> >> +static int pac_dns_resolve(duk_context *ctx) {
> >> +_cleanup_free_ char *address = NULL;
> >> +struct addrinfo hints = {};
> >> +struct addrinfo *res, *addr;
> >> +const char *hostname;
> >> +int r;
> >> +
> >> +hostname = duk_require_string(ctx, 0);
> >> +
> >> +hints.ai_family = AF_INET;
> >> +
> >> +r = getaddrinfo(hostname, NULL, &hints, &res);
> >> +if (r != 0)
> >> +return 0;
> >
> > Hm, synchronous getaddrinfo() is nasty... Please use sd-resolve for
> > this, which adds asynchronous getaddrinfo() for cases like this...
>
> you do realize that you want this synchronous. These are the magic
> dnsResolve and myIpAddress Javascript functions that are expected to
> be present when executing the PAC file. There are a few more, but
> they can be implemented in Javascript and don't need a C
> backend. These two actually need that. So you need to report the
> result.
Ah I see, this function is only ever called from the PAC javascript
code? OK, in that case it should be synchronous indeed.
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC 3/6] proxy-discoveryd: Add PAC support through duktape js engine
Hi Lennart,
>> +struct PAC {
>> +duk_context *ctx;
>> +};
>> +
>> +static int get_addresses_from_interface(int ifindex, union in_addr_union
>> *address) {
>> +struct ifreq ifr = {};
>> +int sk;
>> +
>> +sk = socket(AF_INET, SOCK_DGRAM, 0);
>> +if (sk < 0)
>> +return -1;
>
> No made up errors please! Return -errno or so.
>
>> +
>> +ifr.ifr_ifindex = ifindex;
>> +
>> +if (ioctl(sk, SIOCGIFNAME, &ifr) < 0 || ioctl(sk, SIOCGIFADDR,
>> &ifr) < 0) {
>> +close(sk);
>> +return -1;
>
> Same here...
>
> Also, please don't use the old ioctls for querying network
> information. Use netlink through sd-rtnl. You can look at the
> systemd-resolved sources, they do this already, and this code should
> probably do it very similar from that.
>
>> +static int pac_dns_resolve(duk_context *ctx) {
>> +_cleanup_free_ char *address = NULL;
>> +struct addrinfo hints = {};
>> +struct addrinfo *res, *addr;
>> +const char *hostname;
>> +int r;
>> +
>> +hostname = duk_require_string(ctx, 0);
>> +
>> +hints.ai_family = AF_INET;
>> +
>> +r = getaddrinfo(hostname, NULL, &hints, &res);
>> +if (r != 0)
>> +return 0;
>
> Hm, synchronous getaddrinfo() is nasty... Please use sd-resolve for
> this, which adds asynchronous getaddrinfo() for cases like this...
you do realize that you want this synchronous. These are the magic dnsResolve
and myIpAddress Javascript functions that are expected to be present when
executing the PAC file. There are a few more, but they can be implemented in
Javascript and don't need a C backend. These two actually need that. So you
need to report the result.
Regards
Marcel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
