5:43 a.m.
This is not secure so should not be used routinely. Also we do not
attempt to filter environment variables, so even ones containing
multiple lines or special characters are all sent to nbdkit_debug.
The reason for adding this is to allow for debugging the new
nbd_set_socket_activation_name(3) API added in libnbd 1.16.
---
docs/nbdkit.pod | 10 ++++++++++
server/main.c | 28 ++++++++++++++++++++++++++++
2 files changed, 38 insertions(+)
diff --git a/docs/nbdkit.pod b/docs/nbdkit.pod
index 83cf6a11c..22350cc08 100644
--- a/docs/nbdkit.pod
+++ b/docs/nbdkit.pod
@@ -619,6 +619,16 @@ are numerous and not usually very interesting.
S<I<-D nbdkit.backend.controlpath=0>> suppresses the non-datapath
commands (config, open, close, can_write, etc.)
+=item B<-D nbdkit.environ=1>
+
+(not Windows)
+
+Print nbdkit's environment variables in the debug output at start up.
+This is insecure because environment variables may contain both
+sensitive and user-controlled information, so it should not be used
+routinely. But it is useful for tracking down problems related to
+environment variables.
+
=item B<-D nbdkit.tls.log=>N
Enable TLS logging. C<N> can be in the range 0 (no logging) to 99.
diff --git a/server/main.c b/server/main.c
index 1df5d69ac..7b6adc3cc 100644
--- a/server/main.c
+++ b/server/main.c
@@ -210,6 +210,26 @@ dump_config (void)
#endif
}
+#ifndef WIN32
+
+/* -D nbdkit.environ=1 to dump the environment at start up. */
+NBDKIT_DLL_PUBLIC int nbdkit_debug_environ;
+
+#ifndef HAVE_ENVIRON_DECL
+extern char **environ;
+#endif
+
+static void
+dump_environment (void)
+{
+ size_t i;
+
+ for (i = 0; environ[i]; ++i)
+ nbdkit_debug ("%s", environ[i]);
+}
+
+#endif /* !WIN32 */
+
int
main (int argc, char *argv[])
{
@@ -662,6 +682,14 @@ main (int argc, char *argv[])
/* Check all debug flags were used, and free them. */
free_debug_flags ();
+#ifndef WIN32
+ /* Dump the environment if asked. This is the earliest we can do it
+ * because it uses a debug flag.
+ */
+ if (nbdkit_debug_environ && verbose)
+ dump_environment ();
+#endif
+
if (help) {
struct backend *b;
--
2.39.2
1:54 a.m.
On 5/7/23 12:43, Richard W.M. Jones wrote:
This is not secure so should not be used routinely. Also we do not
attempt to filter environment variables, so even ones containing
multiple lines or special characters are all sent to nbdkit_debug.
The reason for adding this is to allow for debugging the new
nbd_set_socket_activation_name(3) API added in libnbd 1.16.
---
docs/nbdkit.pod | 10 ++++++++++
server/main.c | 28 ++++++++++++++++++++++++++++
2 files changed, 38 insertions(+)
diff --git a/docs/nbdkit.pod b/docs/nbdkit.pod
index 83cf6a11c..22350cc08 100644
--- a/docs/nbdkit.pod
+++ b/docs/nbdkit.pod
@@ -619,6 +619,16 @@ are numerous and not usually very interesting.
S<I<-D nbdkit.backend.controlpath=0>> suppresses the non-datapath
commands (config, open, close, can_write, etc.)
+=item B<-D nbdkit.environ=1>
+
+(not Windows)
+
+Print nbdkit's environment variables in the debug output at start up.
+This is insecure because environment variables may contain both
+sensitive and user-controlled information, so it should not be used
+routinely. But it is useful for tracking down problems related to
+environment variables.
+
=item B<-D nbdkit.tls.log=>N
Enable TLS logging. C<N> can be in the range 0 (no logging) to 99.
diff --git a/server/main.c b/server/main.c
index 1df5d69ac..7b6adc3cc 100644
--- a/server/main.c
+++ b/server/main.c
@@ -210,6 +210,26 @@ dump_config (void)
#endif
}
+#ifndef WIN32
+
+/* -D nbdkit.environ=1 to dump the environment at start up. */
+NBDKIT_DLL_PUBLIC int nbdkit_debug_environ;
+
+#ifndef HAVE_ENVIRON_DECL
+extern char **environ;
+#endif
+
+static void
+dump_environment (void)
+{
+ size_t i;
+
+ for (i = 0; environ[i]; ++i)
+ nbdkit_debug ("%s", environ[i]);
+}
+
+#endif /* !WIN32 */
+
int
main (int argc, char *argv[])
{
@@ -662,6 +682,14 @@ main (int argc, char *argv[])
/* Check all debug flags were used, and free them. */
free_debug_flags ();
+#ifndef WIN32
+ /* Dump the environment if asked. This is the earliest we can do it
+ * because it uses a debug flag.
+ */
+ if (nbdkit_debug_environ && verbose)
+ dump_environment ();
+#endif
+
if (help) {
struct backend *b;
I was surprised not to see any assignment to the new global variable,
but then I found "server/debug-flags.c"; that's some serious wizardry.
Side thought: should we not make both "nbdkit_debug_environ" (and
friends), and the target of the "sym" pointer in "apply_debug_flags",
volatile? Aliasing doesn't get much more obscure than this, and I wonder
if this has the potential to blow up later, when compilers (and
processors) get even more clever.
Of course POSIX could always say "this is just required to work", which
I guess would be fine, but currently it doesn't seem to say it.
Anyway:
Acked-by: Laszlo Ersek <lersek(a)redhat.com>
Thanks for writing the patch!
Laszlo
4:33 p.m.
On Mon, May 08, 2023 at 08:54:48AM +0200, Laszlo Ersek wrote:
On 5/7/23 12:43, Richard W.M. Jones wrote:
> This is not secure so should not be used routinely. Also we do not
> attempt to filter environment variables, so even ones containing
> multiple lines or special characters are all sent to nbdkit_debug.
>
> The reason for adding this is to allow for debugging the new
> nbd_set_socket_activation_name(3) API added in libnbd 1.16.
> ---
> docs/nbdkit.pod | 10 ++++++++++
> server/main.c | 28 ++++++++++++++++++++++++++++
> 2 files changed, 38 insertions(+)
>
> diff --git a/docs/nbdkit.pod b/docs/nbdkit.pod
> index 83cf6a11c..22350cc08 100644
> --- a/docs/nbdkit.pod
> +++ b/docs/nbdkit.pod
> @@ -619,6 +619,16 @@ are numerous and not usually very interesting.
> S<I<-D nbdkit.backend.controlpath=0>> suppresses the non-datapath
> commands (config, open, close, can_write, etc.)
>
> +=item B<-D nbdkit.environ=1>
> +
> +(not Windows)
> +
> +Print nbdkit's environment variables in the debug output at start up.
> +This is insecure because environment variables may contain both
> +sensitive and user-controlled information, so it should not be used
> +routinely. But it is useful for tracking down problems related to
> +environment variables.
> +
> =item B<-D nbdkit.tls.log=>N
>
> Enable TLS logging. C<N> can be in the range 0 (no logging) to 99.
> diff --git a/server/main.c b/server/main.c
> index 1df5d69ac..7b6adc3cc 100644
> --- a/server/main.c
> +++ b/server/main.c
> @@ -210,6 +210,26 @@ dump_config (void)
> #endif
> }
>
> +#ifndef WIN32
> +
> +/* -D nbdkit.environ=1 to dump the environment at start up. */
> +NBDKIT_DLL_PUBLIC int nbdkit_debug_environ;
> +
> +#ifndef HAVE_ENVIRON_DECL
> +extern char **environ;
> +#endif
> +
> +static void
> +dump_environment (void)
> +{
> + size_t i;
> +
> + for (i = 0; environ[i]; ++i)
> + nbdkit_debug ("%s", environ[i]);
> +}
> +
> +#endif /* !WIN32 */
> +
> int
> main (int argc, char *argv[])
> {
> @@ -662,6 +682,14 @@ main (int argc, char *argv[])
> /* Check all debug flags were used, and free them. */
> free_debug_flags ();
>
> +#ifndef WIN32
> + /* Dump the environment if asked. This is the earliest we can do it
> + * because it uses a debug flag.
> + */
> + if (nbdkit_debug_environ && verbose)
> + dump_environment ();
> +#endif
> +
> if (help) {
> struct backend *b;
>
I was surprised not to see any assignment to the new global variable,
but then I found "server/debug-flags.c"; that's some serious wizardry.
With the side effect, as it turns out, that it's very hard to
implement for language plugins. Rust is fine. OCaml can be done by
adding a C object to the link. Other languages, not really :-(
Side thought: should we not make both
"nbdkit_debug_environ" (and
friends), and the target of the "sym" pointer in
"apply_debug_flags",
volatile? Aliasing doesn't get much more obscure than this, and I wonder
if this has the potential to blow up later, when compilers (and
processors) get even more clever.
Of course POSIX could always say "this is just required to work", which
I guess would be fine, but currently it doesn't seem to say it.
I guess Eric will know for sure, but isn't it generally true that all
global variables can be modified by any function call? My model is
that they can modified at any time by any called function; even
through a pointer so not really visible to the compiler.
Anyway:
Acked-by: Laszlo Ersek <lersek(a)redhat.com>
Thanks for writing the patch!
Laszlo
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
4:52 p.m.
On Mon, May 08, 2023 at 10:33:25PM +0100, Richard W.M. Jones wrote:
> >
>
> I was surprised not to see any assignment to the new global variable,
> but then I found "server/debug-flags.c"; that's some serious
wizardry.
With the side effect, as it turns out, that it's very hard to
implement for language plugins. Rust is fine. OCaml can be done by
adding a C object to the link. Other languages, not really :-(
And not today's problem to solve.
> Side thought: should we not make both "nbdkit_debug_environ" (and
> friends), and the target of the "sym" pointer in
"apply_debug_flags",
> volatile? Aliasing doesn't get much more obscure than this, and I wonder
> if this has the potential to blow up later, when compilers (and
> processors) get even more clever.
>
> Of course POSIX could always say "this is just required to work", which
> I guess would be fine, but currently it doesn't seem to say it.
I guess Eric will know for sure, but isn't it generally true that all
global variables can be modified by any function call? My model is
that they can modified at any time by any called function; even
through a pointer so not really visible to the compiler.
Marking a variable as volatile will force the compiler to reload it on
each access, which can slow down loops that read the debug variable.
But in general, these are not variables we are frequently changing (we
do it at most once, during startup, based on the environment); and a
compiler can't completely eliminate global variables unless it is
doing some heavy LTO. I see no reason why we would want to mark it
volatile, because the only thing that would guarantee is that the
compiler no longer hoists repeated reads of the globals outside of
loops, even though our loops won't be changing the debug state.
I suppose something like LTO could provide a compiler an opportunity
to say that no .o linked into the final image modifies the variable
directly, which may hurt nbdkit.* debug variables in the main
executable, but I don't see how the compiler could ever optimize away
access to globals in .so plugins (since the very nature of .so's is
that they will be dynamically run by code that the compiler didn't
see). This is all waaaaay outside of the bounds of what POSIX says,
so at best we are just relying on "does it work for how we are using
it" - but since we are only using it for debugging, where it is doing
what we want, I'm not too worried about trying to make this comply
with some unwritten spec. Sometimes, looking the other way when there
is dark magic present is the prudent action.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
2:12 a.m.
On 5/8/23 23:52, Eric Blake wrote:
On Mon, May 08, 2023 at 10:33:25PM +0100, Richard W.M. Jones wrote:
>>>
>>
>> I was surprised not to see any assignment to the new global variable,
>> but then I found "server/debug-flags.c"; that's some serious
wizardry.
>
> With the side effect, as it turns out, that it's very hard to
> implement for language plugins. Rust is fine. OCaml can be done by
> adding a C object to the link. Other languages, not really :-(
And not today's problem to solve.
>
>> Side thought: should we not make both "nbdkit_debug_environ" (and
>> friends), and the target of the "sym" pointer in
"apply_debug_flags",
>> volatile? Aliasing doesn't get much more obscure than this, and I wonder
>> if this has the potential to blow up later, when compilers (and
>> processors) get even more clever.
>>
>> Of course POSIX could always say "this is just required to work",
which
>> I guess would be fine, but currently it doesn't seem to say it.
[*]
>
> I guess Eric will know for sure, but isn't it generally true that all
> global variables can be modified by any function call? My model is
> that they can modified at any time by any called function; even
> through a pointer so not really visible to the compiler.
Marking a variable as volatile will force the compiler to reload it on
each access, which can slow down loops that read the debug variable.
But in general, these are not variables we are frequently changing (we
do it at most once, during startup, based on the environment); and a
compiler can't completely eliminate global variables unless it is
doing some heavy LTO. I see no reason why we would want to mark it
volatile, because the only thing that would guarantee is that the
compiler no longer hoists repeated reads of the globals outside of
loops, even though our loops won't be changing the debug state.
Using "nbdkit_debug_environ" as an example:
The entire source code contains no assignment to "nbdkit_debug_environ",
not even through a pointer derived from "nbdkit_debug_environ". A super
smart compiler might validly assume that "nbdkit_debug_environ" is never
written to, therefore replace all read accesses to
"nbdkit_debug_environ" with constants (and open-code or remove the
dependent, conditional code).
So there is a reason why we *might* want to mark the variable as
volatile: compilers are getting too smart. I'm not saying it is
*practical*; in fact I'm sure dlopen() and dlsym() *guarantee* that
Rich's code will work as intended, as written. I guess I'm just asking
for POSIX to spell that out.
... and, in retrospect, it does! So my statement at [*] was wrong! The
dlsym() spec includes:
The return value from dlsym(), cast to a pointer to the type of the
named symbol, can be used to call (in the case of a function) or
access the contents of (in the case of a data object) the named
symbol.
OK! This is all I wanted to see.
(This language covers, for example, the inevitable requirement that the
I-Cache be flushed somewhere inside dlopen() and/or dlsym(); otherwise a
just-loaded function couldn't be called safely!)
I suppose something like LTO could provide a compiler an opportunity
to say that no .o linked into the final image modifies the variable
directly, which may hurt nbdkit.* debug variables in the main
executable, but I don't see how the compiler could ever optimize away
access to globals in .so plugins (since the very nature of .so's is
that they will be dynamically run by code that the compiler didn't
see).
I don't like arguments of this sort. They all seem very logical, even
obvious... until the next major release of gcc or clang, that is. It's
not safe (albeit practical) when our arguments are limited by our own
imagination. I've lost trust in the direction in which compilers have
been developed.
This is all waaaaay outside of the bounds of what POSIX says,
In my opinion, it isn't; I did end up finding the important passage that
I missed (or failed to recall) earlier!
so at best we are just relying on "does it work for how we are
using
it" - but since we are only using it for debugging, where it is doing
what we want, I'm not too worried about trying to make this comply
with some unwritten spec. Sometimes, looking the other way when there
is dark magic present is the prudent action.
I do agree with your last statement 100%, in general!
Anyway, just to confirm: thanks for talking this through with me, I
agree that the code is safe, and it's even based on clear POSIX standardese.
Thanks!
Laszlo
1:56 a.m.
On 5/8/23 23:33, Richard W.M. Jones wrote:
On Mon, May 08, 2023 at 08:54:48AM +0200, Laszlo Ersek wrote:
> On 5/7/23 12:43, Richard W.M. Jones wrote:
>> This is not secure so should not be used routinely. Also we do not
>> attempt to filter environment variables, so even ones containing
>> multiple lines or special characters are all sent to nbdkit_debug.
>>
>> The reason for adding this is to allow for debugging the new
>> nbd_set_socket_activation_name(3) API added in libnbd 1.16.
>> ---
>> docs/nbdkit.pod | 10 ++++++++++
>> server/main.c | 28 ++++++++++++++++++++++++++++
>> 2 files changed, 38 insertions(+)
>>
>> diff --git a/docs/nbdkit.pod b/docs/nbdkit.pod
>> index 83cf6a11c..22350cc08 100644
>> --- a/docs/nbdkit.pod
>> +++ b/docs/nbdkit.pod
>> @@ -619,6 +619,16 @@ are numerous and not usually very interesting.
>> S<I<-D nbdkit.backend.controlpath=0>> suppresses the non-datapath
>> commands (config, open, close, can_write, etc.)
>>
>> +=item B<-D nbdkit.environ=1>
>> +
>> +(not Windows)
>> +
>> +Print nbdkit's environment variables in the debug output at start up.
>> +This is insecure because environment variables may contain both
>> +sensitive and user-controlled information, so it should not be used
>> +routinely. But it is useful for tracking down problems related to
>> +environment variables.
>> +
>> =item B<-D nbdkit.tls.log=>N
>>
>> Enable TLS logging. C<N> can be in the range 0 (no logging) to 99.
>> diff --git a/server/main.c b/server/main.c
>> index 1df5d69ac..7b6adc3cc 100644
>> --- a/server/main.c
>> +++ b/server/main.c
>> @@ -210,6 +210,26 @@ dump_config (void)
>> #endif
>> }
>>
>> +#ifndef WIN32
>> +
>> +/* -D nbdkit.environ=1 to dump the environment at start up. */
>> +NBDKIT_DLL_PUBLIC int nbdkit_debug_environ;
>> +
>> +#ifndef HAVE_ENVIRON_DECL
>> +extern char **environ;
>> +#endif
>> +
>> +static void
>> +dump_environment (void)
>> +{
>> + size_t i;
>> +
>> + for (i = 0; environ[i]; ++i)
>> + nbdkit_debug ("%s", environ[i]);
>> +}
>> +
>> +#endif /* !WIN32 */
>> +
>> int
>> main (int argc, char *argv[])
>> {
>> @@ -662,6 +682,14 @@ main (int argc, char *argv[])
>> /* Check all debug flags were used, and free them. */
>> free_debug_flags ();
>>
>> +#ifndef WIN32
>> + /* Dump the environment if asked. This is the earliest we can do it
>> + * because it uses a debug flag.
>> + */
>> + if (nbdkit_debug_environ && verbose)
>> + dump_environment ();
>> +#endif
>> +
>> if (help) {
>> struct backend *b;
>>
>
> I was surprised not to see any assignment to the new global variable,
> but then I found "server/debug-flags.c"; that's some serious wizardry.
With the side effect, as it turns out, that it's very hard to
implement for language plugins. Rust is fine. OCaml can be done by
adding a C object to the link. Other languages, not really :-(
Hm. I didn't even realize it was being done like this for plugins' sake.
I assumed it was there to avoid boilerplate code.
I figure an alternative would be for -D to stash the flags in a
dictionary (or list) in the main program, and then each plugin / filter
could access that data structure (or even get a reference to it in (one
of) the entry point function(s)).
IIRC, if you build the main program with -rdynamic, then dynamically
loaded objects can use the main program for resolving their symbols...
Indeed, nbdkit already seems to use -rdynamic; at least "configure.ac"
checks for it.
> Side thought: should we not make both "nbdkit_debug_environ" (and
> friends), and the target of the "sym" pointer in
"apply_debug_flags",
> volatile? Aliasing doesn't get much more obscure than this, and I wonder
> if this has the potential to blow up later, when compilers (and
> processors) get even more clever.
>
> Of course POSIX could always say "this is just required to work", which
> I guess would be fine, but currently it doesn't seem to say it.
I guess Eric will know for sure, but isn't it generally true that all
global variables can be modified by any function call? My model is
that they can modified at any time by any called function; even
through a pointer so not really visible to the compiler.
Well to express it with some hand-waving, modifying globals through
pointers are of course fine, but it's about "pointer provenance". The
dlsym() use here isn't much different from reading a pointer from a
FILE* with fscanf("%p"), casting the result to int*, then de-referencing
the result. Collecting the addresses of global variables in a list and
passing the list to another function (even in a dl-opened object) is
different.
Just because the object is a global variable, concerns are not
immmediately removed. For example, per POSIX, if you get a signal
delivered (truly) asynchronously, you can only mostly assign a value to
a "volatile sig_atomic_t" global variable in the signal catching
function. (I'm not being exact here, see the spec for details.) The spec
doesn't explain (as far as I understand), but the "sig_atomic_t" type is
required so that no matter at what instruciton boundary the signal is
delivered, the variable can never be modified halfway, and the
"volatile" is there for the same visibility concern that I'm raising here.
Compilers have been getting more aggressive in exploiting the "strict
aliasing" rules; that was all I wanted to raise.
I'll comment some more under Eric's response.
Thanks!
Laszlo
> Anyway:
>
> Acked-by: Laszlo Ersek <lersek(a)redhat.com>
>
> Thanks for writing the patch!
> Laszlo
Rich.
4:47 a.m.
On Tue, May 09, 2023 at 08:56:47AM +0200, Laszlo Ersek wrote:
On 5/8/23 23:33, Richard W.M. Jones wrote:
> On Mon, May 08, 2023 at 08:54:48AM +0200, Laszlo Ersek wrote:
>> On 5/7/23 12:43, Richard W.M. Jones wrote:
>>> This is not secure so should not be used routinely. Also we do not
>>> attempt to filter environment variables, so even ones containing
>>> multiple lines or special characters are all sent to nbdkit_debug.
>>>
>>> The reason for adding this is to allow for debugging the new
>>> nbd_set_socket_activation_name(3) API added in libnbd 1.16.
>>> ---
>>> docs/nbdkit.pod | 10 ++++++++++
>>> server/main.c | 28 ++++++++++++++++++++++++++++
>>> 2 files changed, 38 insertions(+)
>>>
>>> diff --git a/docs/nbdkit.pod b/docs/nbdkit.pod
>>> index 83cf6a11c..22350cc08 100644
>>> --- a/docs/nbdkit.pod
>>> +++ b/docs/nbdkit.pod
>>> @@ -619,6 +619,16 @@ are numerous and not usually very interesting.
>>> S<I<-D nbdkit.backend.controlpath=0>> suppresses the
non-datapath
>>> commands (config, open, close, can_write, etc.)
>>>
>>> +=item B<-D nbdkit.environ=1>
>>> +
>>> +(not Windows)
>>> +
>>> +Print nbdkit's environment variables in the debug output at start up.
>>> +This is insecure because environment variables may contain both
>>> +sensitive and user-controlled information, so it should not be used
>>> +routinely. But it is useful for tracking down problems related to
>>> +environment variables.
>>> +
>>> =item B<-D nbdkit.tls.log=>N
>>>
>>> Enable TLS logging. C<N> can be in the range 0 (no logging) to 99.
>>> diff --git a/server/main.c b/server/main.c
>>> index 1df5d69ac..7b6adc3cc 100644
>>> --- a/server/main.c
>>> +++ b/server/main.c
>>> @@ -210,6 +210,26 @@ dump_config (void)
>>> #endif
>>> }
>>>
>>> +#ifndef WIN32
>>> +
>>> +/* -D nbdkit.environ=1 to dump the environment at start up. */
>>> +NBDKIT_DLL_PUBLIC int nbdkit_debug_environ;
>>> +
>>> +#ifndef HAVE_ENVIRON_DECL
>>> +extern char **environ;
>>> +#endif
>>> +
>>> +static void
>>> +dump_environment (void)
>>> +{
>>> + size_t i;
>>> +
>>> + for (i = 0; environ[i]; ++i)
>>> + nbdkit_debug ("%s", environ[i]);
>>> +}
>>> +
>>> +#endif /* !WIN32 */
>>> +
>>> int
>>> main (int argc, char *argv[])
>>> {
>>> @@ -662,6 +682,14 @@ main (int argc, char *argv[])
>>> /* Check all debug flags were used, and free them. */
>>> free_debug_flags ();
>>>
>>> +#ifndef WIN32
>>> + /* Dump the environment if asked. This is the earliest we can do it
>>> + * because it uses a debug flag.
>>> + */
>>> + if (nbdkit_debug_environ && verbose)
>>> + dump_environment ();
>>> +#endif
>>> +
>>> if (help) {
>>> struct backend *b;
>>>
>>
>> I was surprised not to see any assignment to the new global variable,
>> but then I found "server/debug-flags.c"; that's some serious
wizardry.
>
> With the side effect, as it turns out, that it's very hard to
> implement for language plugins. Rust is fine. OCaml can be done by
> adding a C object to the link. Other languages, not really :-(
Hm. I didn't even realize it was being done like this for plugins' sake.
I assumed it was there to avoid boilerplate code.
Yes it's there to avoid boilerplate code. You can create a debug
variable in a plugin very simply by exporting a symbol with the
<pluginname>_debug_<name> pattern.
I figure an alternative would be for -D to stash the flags in a
dictionary (or list) in the main program, and then each plugin / filter
could access that data structure (or even get a reference to it in (one
of) the entry point function(s)).
We can't really change how it works now as it is baked into the ABI.
v2 coming up ...
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
8:35 a.m.
On Sun, May 07, 2023 at 11:43:17AM +0100, Richard W.M. Jones wrote:
This is not secure so should not be used routinely. Also we do not
attempt to filter environment variables, so even ones containing
multiple lines or special characters are all sent to nbdkit_debug.
The reason for adding this is to allow for debugging the new
nbd_set_socket_activation_name(3) API added in libnbd 1.16.
---
+static void
+dump_environment (void)
+{
+ size_t i;
+
+ for (i = 0; environ[i]; ++i)
+ nbdkit_debug ("%s", environ[i]);
We have shell_quote in common/utils/quote.c; shouldn't we use it here
to avoid ambiguities when an env-var contains newline?
+}
+
+#endif /* !WIN32 */
+
int
main (int argc, char *argv[])
{
@@ -662,6 +682,14 @@ main (int argc, char *argv[])
/* Check all debug flags were used, and free them. */
free_debug_flags ();
+#ifndef WIN32
+ /* Dump the environment if asked. This is the earliest we can do it
+ * because it uses a debug flag.
+ */
+ if (nbdkit_debug_environ && verbose)
+ dump_environment ();
Why does this not work on WIN32?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
4:26 p.m.
On Mon, May 08, 2023 at 08:35:53AM -0500, Eric Blake wrote:
On Sun, May 07, 2023 at 11:43:17AM +0100, Richard W.M. Jones wrote:
> This is not secure so should not be used routinely. Also we do not
> attempt to filter environment variables, so even ones containing
> multiple lines or special characters are all sent to nbdkit_debug.
>
> The reason for adding this is to allow for debugging the new
> nbd_set_socket_activation_name(3) API added in libnbd 1.16.
> ---
> +static void
> +dump_environment (void)
> +{
> + size_t i;
> +
> + for (i = 0; environ[i]; ++i)
> + nbdkit_debug ("%s", environ[i]);
We have shell_quote in common/utils/quote.c; shouldn't we use it here
to avoid ambiguities when an env-var contains newline?
I was wondering more generally about this. I actually think that
nbdkit_debug ought to be more tolerant of unusual inputs -- not least
because there are potential security concerns. All kinds of random
user-controlled strings are sent to nbdkit_debug and it just dumps it
to stderr entirely unfiltered. So rather than fixing this one
instance, we should fix nbdkit_debug. I'll look at a follow-up patch.
> +}
> +
> +#endif /* !WIN32 */
> +
> int
> main (int argc, char *argv[])
> {
> @@ -662,6 +682,14 @@ main (int argc, char *argv[])
> /* Check all debug flags were used, and free them. */
> free_debug_flags ();
>
> +#ifndef WIN32
> + /* Dump the environment if asked. This is the earliest we can do it
> + * because it uses a debug flag.
> + */
> + if (nbdkit_debug_environ && verbose)
> + dump_environment ();
Why does this not work on WIN32?
I just assumed it wouldn't, but you're right that it would work.
I'll respin this patch once I've tested what it would do on Windows.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
593
days inactive
595
days old
8 comments
3 participants
participants (3)
-
Eric Blake -
Laszlo Ersek -
Richard W.M. Jones