On 2015/05/19 02:07, Yclept Nemo <orbisvi...@gmail.com> wrote:
> On Mon, May 18, 2015 at 5:27 PM, Ben Boeckel <maths...@gmail.com> wrote:
>
> > Supporting a command (for tools such as pass[1]) would also be good.
> > Also, is there any additional documentation along with this?
> >
>
> So a password_backend ("command") that runs a command and retrieves stdout
> and return code? Any recommendations boost, popen, etc?
That was one thing I didn't agree with. I wouldn't do that.
> This (mpd.secret.py) is not a hard runtime dependency; it is a script to
> help end-users setup libsecret keyrings.
So this is not MPD specific? Is there no other existing tool to do
that? If not, why don't you submit your tool to the libsecret
project? I don't think this belongs here.
> > diff --git a/src/config/ConfigOption.hxx b/src/config/ConfigOption.hxx
> > > index 5acd6fd..8d11a3f 100644
> > > --- a/src/config/ConfigOption.hxx
> > > +++ b/src/config/ConfigOption.hxx
> > > @@ -90,6 +90,7 @@ enum class ConfigBlockOption {
> > > AUDIO_FILTER,
> > > DATABASE,
> > > NEIGHBORS,
> > > + BACKEND,
> >
> > A more descriptive name for this would be better...
> >
>
> I disagree, the BACKEND is intended to be a single top-level keyword to
> configure multiple backends. So the backend block can configure the
> smbclient backend (via name param), in which case the password_backend
> param is specific to the smbclient backend. This makes sense as different
> backends may have different password/attribute requirements, such as a
> hypothetical sftp backend.
I don't like that name either, but I wouldn't make it
"password"-specific. Unless we create a new infrastructure for
"password plugins", that is currently only used by the "smbclient"
family of plugins.
In MPD, "smbclient" is the name for three plugins: "storage", "input"
and "neighbor". But all of the three share some common configuration
- of which there is currently none, and there is no way of configuring
a family of plugins. You could add configuration to an "input" block,
or to a "neighbor" block (there is no "storage" block, storage
configuration consists of nothing but a single URI string).
Choosing either of the two "input" or "neighbor" is confusing because
it affects all three "smbclient" plugins. So there must be a new
top-level block keyword.
"backend" is a lousy choice, but I can't come up with a better one
right now. I thought of "subsystem" but then again, "smbclient" is
not a subsystem, but a family of MPD plugins.
Maybe just "plugin" and its settings are auto-merged into all plugin
configuration blocks with the same name?
> > +#pragma GCC diagnostic push
> > > +#pragma GCC diagnostic ignored "-Wmissing-field-initializers"
> >
> > Not everything is GCC. I'd say this should be wrapped in __GNUC__
> > checks.
And not all gcc versions support this #pragma!
We have a few helper macros in Compiler.h.
But why not fix these warnings in the first place?
> > +#ifdef ENABLE_LIBSECRET
> > > +const char*
> > > +mpd_smbc_get_auth_data_secret
> > > + ( SmbAuthData* smb_auth_data
> > > + )
> > > {
> > > - // TODO: implement
> > > - strcpy(wg, "WORKGROUP");
> > > - strcpy(un, "");
> > > - strcpy(pw, "");
> > > + GError *error = NULL;
> > > +
> > > + // When to free?
> >
> > The caller should probably own the data returned here (other backends
> > use strdup for valid values, and NULL be used on error paths).
If the value is a "const" pointer, the caller doesn't own it, because
you can't "free" a const pointer.
But it's right, the semantics of ownership must be well-defined.
> I'm not really sure what you mean by other backends? I took a cursory look
> at:
> * (ConfigBlock*)config_get_block(...);
> * (...)GetBlockValue(...);
> In both cases the data returned is not owned by the caller. Correct me if
> I'm wrong, but I'm not supposed to delete/free this data?
Because these functions return pointers into a larger object (the
global configuration). They are just looking up existing objects.
> That said, I'll probably add a bool value to SmbAuthData indicating the
> password field needs to be destroyed via secret_password_free.
Uhh. That sounds like awful complexity.
> > > + const char* password = (const char*)secret_password_lookup_sync
> >
> > Should the event loop really be blocked? Also, are collections and/or
> > items unlocked here properly?
> >
>
> Yeah wasn't really sure about this. My reasoning is, why not block if the
> music_directory isn't even available yet. Also, I wasn't sure how to hook
> into the event loop.
If the music_directory isn't available yet, you can still listen to
radio.
The problem is that libsmbclient is a purely synchronous library, and
that's a big problem for MPD. Not a good choice, but the only one we
have. (I'd love to switch to a different library; is there one?)
Max
_______________________________________________
mpd-devel mailing list
mpd-devel@musicpd.org
http://mailman.blarg.de/listinfo/mpd-devel
