>
> > 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.
>
Well it is both mpd-specific in the labelling
* schema name: org.mpd.smbclient.password
* collection label: MusicPD
And smbclient specific in
* password label: Smbclient Password: {server}
* schema attributes
There is a tool secret-tool, but it doesn't allow for:
* collection creation
* storing a secret in a specific collection
This tool does both, and automates the process specifically for mpd keyring
users.
> > 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?
>
or "library"?
>
> > > +#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?
>
As used this diagnostic requires GCC >= 4.6, but this is a global mpd
requirement so I won't version. From Compiler.h:
....error Sorry, your gcc version is too old. You need at least version 4.6.
SecretSchema is (an opaque?) data structure with reserved fields
SecretSchema::reserved1-reserved12. If this fields aren't used GCC
generates some pretty ugly warnings.
> > > +#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.
>
IMO if you malloc data and cast to const you should be able to free it; can
free by recasting as non-const.
> But it's right, the semantics of ownership must be well-defined.
>
Not sure what you mean by well-defined, there are no comments indicating
ownership.
> 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.
>
Right, message received. Don't free these. Makes sense.
> > 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.
>
I can (will) wrap this into a private field and a destructor to make it
more palatable...
> > > > + 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.
>
Well, some problems in making libsecret async:
1] Integrate libsecret's mainloop (glib) into mpd's (chosen during
configure?) or vice versa.
2] Entry point for the async return callback can't be
mpd_smbc_get_auth_data (1st arg to smbc_init) because that's already a
callback and smbclient will initialize as soon as it returns.
So it seems the async point would have to at SmbclientInit() or higher, and
any function that calls this (storage, input, neighbor) would have to be
split with a return async callback. Thing then get complicated with locking
the use of any smbclient-mpd related datastructures until the callback is
called. High complexity for minimal returns (listening 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?)
Cursory google for async smbclient returned nothing, but I think the
synchronous code-paths can be reduced. Every new storage open() (play new
song) calls smbc_init, I think this is not required. I'm pretty sure
smbc_init initializes the workgroup/username/password fields as global
libsmbclient variables, this function need only be called once. I'll add a
static (c-style) global struct to smbclient/Init.cxx tracking
smbc_init_called state.
This would make libsecret synchronous code only affect the first smbclient
connection. Does this still affect playing radio? SmbclientInit is called
while reading the configuration, ie during program startup. Consider it
necessary startup time?
_______________________________________________
mpd-devel mailing list
mpd-devel@musicpd.org
http://mailman.blarg.de/listinfo/mpd-devel
