[vlc-devel] [PATCH] cloudstorage module, with folders browsing and keystore
Filip Roséen
filip at atch.se
Mon Apr 24 22:04:00 CEST 2017
Hi William,
I am writing this in the back of a cab, so I might miss obvious stuff
and/or misread something because of the slightly bumpy ride. If
anything is unclear, or just utterly wrong, I apologize.
On 2017-04-19 16:24, William Ung wrote:
> ---
> configure.ac | 2 +-
> include/vlc_keystore.h | 7 +-
> modules/access/Makefile.am | 7 ++
> modules/access/cloudstorage.cpp | 264 ++++++++++++++++++++++++++++++++++++++++
> src/libvlccore.sym | 1 +
> src/misc/keystore.c | 4 +-
> 6 files changed, 280 insertions(+), 5 deletions(-)
> create mode 100644 modules/access/cloudstorage.cpp
>
> diff --git a/configure.ac b/configure.ac
> index fde2dea..a0538a1 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -2087,7 +2087,7 @@ dnl
> dnl matroska demux plugin
> dnl
> PKG_ENABLE_MODULES_VLC([MATROSKA], [mkv], [libebml libmatroska], [MKV format support], [auto])
> -
> +PKG_ENABLE_MODULES_VLC([CLOUDSTORAGE], [cloudstorage], [libcloudstorage], [cloudstorage access], [auto])
> dnl
> dnl modplug demux plugin
> dnl
> diff --git a/include/vlc_keystore.h b/include/vlc_keystore.h
> index 0af15eb..d081877 100644
> --- a/include/vlc_keystore.h
> +++ b/include/vlc_keystore.h
> @@ -15,7 +15,7 @@
> *
> * You should have received a copy of the GNU Lesser General Public License
> * along with this program; if not, write to the Free Software Foundation,
> - * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.
> + * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.obj
> *****************************************************************************/
>
> #ifndef VLC_KEYSTORE_H
> @@ -72,6 +72,9 @@ struct vlc_keystore_entry
> size_t i_secret_len;
> };
>
> +VLC_API vlc_keystore *
> +get_memory_keystore(vlc_object_t *p_parent);
> +
> /**
> * Create a keystore object
> *
> @@ -117,7 +120,7 @@ vlc_keystore_store(vlc_keystore *p_keystore,
>
> /**
> * Find all entries that match a set of key/values
> - *
> + *
> * @param ppsz_values set of key/values, see vlc_keystore_key, any values can
> * be NULL
> * @param pp_entries list of found entries. To be released with
> diff --git a/modules/access/Makefile.am b/modules/access/Makefile.am
> index dd485a2..6b21a90 100644
> --- a/modules/access/Makefile.am
> +++ b/modules/access/Makefile.am
> @@ -313,6 +313,13 @@ endif
>
> ### Network streams ###
>
> +libcloudstorage_plugin_la_SOURCES = access/cloudstorage/cloudstorage.cpp access/cloudstorage/cloudstorage.h access/cloudstorage/callback.h
> +libcloudstorage_plugin_la_CXXFLAGS = $(CXXFLAGS_cloudstorage) $(AM_CXXFLAGS)
> +libcloudstorage_plugin_la_LIBADD = $(LIBS_cloudstorage)
> +libcloudstorage_plugin_la_LDFLAGS = $(AM_LDFLAGS) -rpath '$(accessdir)'
> +access_LTLIBRARIES += $(LTLIBcloudstorage)
> +EXTRA_LTLIBRARIES += libcloudstorage_plugin.la
> +
> libftp_plugin_la_SOURCES = access/ftp.c
> libftp_plugin_la_LIBADD = $(SOCKET_LIBS)
> access_LTLIBRARIES += libftp_plugin.la
> diff --git a/modules/access/cloudstorage.cpp b/modules/access/cloudstorage.cpp
> new file mode 100644
> index 0000000..a3ea31d
> --- /dev/null
> +++ b/modules/access/cloudstorage.cpp
> @@ -0,0 +1,264 @@
> +#ifdef HAVE_CONFIG_H
> +#include "config.h"
> +#endif /* HAVE_CONFIG_H */
> +
> +#include <cstdarg>
> +#include <cstdlib>
> +#include <iostream>
> +#include <fstream>
> +#include <mutex>
> +#include <thread>
> +#include <vector>
> +#include <string>
> +#include <memory>
> +#include <condition_variable>
> +
> +#include <ICloudProvider.h>
> +#include <ICloudStorage.h>
> +#include <IItem.h>
> +#include <IRequest.h>
> +
> +#include <vlc_common.h>
> +#include <vlc_plugin.h>
> +#include <vlc_access.h>
> +#include <vlc_services_discovery.h>
> +#include <vlc_keystore.h>
> +
> +using namespace cloudstorage;
> +using cloudstorage::ICloudProvider;
> +using cloudstorage::IDownloadFileCallback;
> +using cloudstorage::IItem;
> +
> +static int Open(vlc_object_t *);
> +static void Close(vlc_object_t *);
> +static int readDir(stream_t *, input_item_node_t *);
> +
> +vlc_module_begin()
> +set_shortname(N_("cloudstorage"))
> +set_capability("access", 0)
> +set_description(N_("cloud input"))
> +set_category(CAT_INPUT)
> +set_subcategory(SUBCAT_INPUT_ACCESS)
> +set_callbacks(Open, Close)
> +
> +vlc_module_end()
> +
> +/*
> +** private struct
> +*/
> +
> +struct access_sys_t
> +{
> + using Pointer = std::shared_ptr<access_sys_t>;
> +
> + ICloudProvider::Pointer provider_;
> + std::string provider_name_;
> + std::string token_;
> +
> + vlc_keystore *p_keystore_;
> + char *ppsz_values[KEY_MAX];
> +
> + IItem::Pointer current_item_;
> + std::vector<std::string> directory_stack_;
> + std::vector<IItem::Pointer> directory_list_;
The above *data-members* seems to always be assigned before being
read, is it required to keep the objects referred to by the
`IItem::Pointer` alive between calls? If so; that should be
documented.
If the above is not true, please replace the usage of this
*data-member* with a variable at the places where it is currently
used.
> +
> + ICloudProvider::ListDirectoryRequest::Pointer list_directory_request_;
> +
> + std::mutex stream_mutex_;
> + std::condition_variable stream_cv_;
These two data-members are not used anywhere in your implementation,
what are there purpose? If there is a need for
*thread-synchronization*, that should be fixed prior to submission.
> +
> + access_sys_t(vlc_object_t *);
> +};
> +
> +/*
> +** Provider Callback
> +*/
> +
> +class Callback : public ICloudProvider::ICallback {
> +public:
> + Callback(access_sys_t *sys)
> + {
> + p_sys = sys;
Please use the *mem-initializer list* instead of plain assignment to
the relevant *data-member*, it is cleaner and strictly speaking more
correct.
> + }
> +
> + Status userConsentRequired(const ICloudProvider& provider) override
> + {
> + std::cout << "User ConsentRequired at : " << provider.authorizeLibraryUrl() << std::endl;
> + return Status::WaitForAuthorizationCode;
Diagnostics should not be printed to `std::cout`, nor any other
standard output-streams. Please use `msg_{Dbg,Warn,Err}` as provided
by VLC.
This comment applies to all occurances of usage of `std::cout`.
> + }
> +
> + void accepted(const ICloudProvider& provider) override
> + {
> + p_sys->token_ = provider.token();
> + vlc_keystore_remove(p_sys->p_keystore_, p_sys->ppsz_values);
> + vlc_keystore_store(p_sys->p_keystore_, p_sys->ppsz_values, (const uint8_t *)p_sys->token_.c_str(), p_sys->token_.size(), p_sys->provider_name_.c_str());
I am aware of the fact that the documentation for `vlc_keystore_store`
might be a little underspecified as it does not explicitly say that
that any previous value will be overwritten, but I'd prefer if the
documentation was updated instead of unconditionally calling
`vlc_keystore_remove` prior to a call to `vlc_keystore_store`.
> + std::cout << "accepted" << std::endl;
> + }
> +
> + void declined(const ICloudProvider& provider) override
> + {
> + std::cout << "declined" << std::endl;
> + }
> +
> + void error(const ICloudProvider& provider, const std::string& desc) override
> + {
> + std::cout << "an error occured : " << desc << std::endl;
> + }
> + access_sys_t *p_sys;
> +};
> +
> +static void get_ppsz_values(char *ppsz_values[KEY_MAX], std::string provider_name)
> +{
> + ppsz_values[KEY_PROTOCOL] = "cloudstorage";
> + ppsz_values[KEY_USER] = "cloudstorage user";
> + ppsz_values[KEY_SERVER] = "cloudstorage";
> +}
There's no point in having a helper-function with `3` lines, that is
only called once; please do manual inlining at the site of call.
> +
> +access_sys_t::access_sys_t(vlc_object_t *p_this)
> +{
> + vlc_keystore_entry *p_entries;
> +
> + p_keystore_ = get_memory_keystore(p_this);
> + provider_name_ = "box";
> + provider_ = cloudstorage::ICloudStorage::create()->provider(provider_name_);
> + if (!provider_)
> + return;
The above seems to indicate that an error shall be propagated, and not
simply premature return; is that accurate? (as there is no call to
`provider_->initialize`, it at least looks weird).
> + VLC_KEYSTORE_VALUES_INIT(ppsz_values);
> + get_ppsz_values(ppsz_values, provider_name_);
> +
> + if (vlc_keystore_find(p_keystore_, ppsz_values, &p_entries) > 0)
> + {
> + std::cout << "cacaaaaa" << '\n';
This is most definitely a left-over from some lazy-man debugging,
but please try to refrain from pushing patches to `vlc-devel` that
include such.
> + token_ = (char *)p_entries[0].p_secret;
> + }
> + provider_->initialize({token_, std::unique_ptr<Callback>(new Callback(this)), nullptr, nullptr, nullptr, {}});
Please remember that exceptions that propagate out from the *"c++
world"* will invoke *undefined-behavior*, as such you will need to
handle cases where an exception may occur, and prevent them from
"leaking" out into the surrounding C world.
Long story, short; you will need *try-catch* clauses in all functions
that are called from the core of VLC (as these shall have C linkage,
and as such are not *"exception friendly"*).
> + current_item_ = provider_->rootDirectory();
> +}
> +
> +static int Control(stream_t *p_access, int i_query, va_list args)
> +{
> + switch (i_query)
> + {
> + case STREAM_IS_DIRECTORY:
> + *va_arg(args, bool *) = p_access->pf_readdir == readDir;
> + break;
> +
> + default:
> + return access_vaDirectoryControlHelper( p_access, i_query, args );
> + }
`switch`-statements with only one explicit label, and then a default,
is most often cleaner written as a plain if-statement. It also makes
more sense for this spceific implementation.
> +
> + return VLC_SUCCESS;
> +}
> +
> +static int add_item(struct access_fsdir *p_fsdir, stream_t *p_access, IItem::Pointer item)
> +{
> + int i_type;
> + access_sys_t *p_sys = (access_sys_t*)p_access->p_sys;
> + ICloudProvider::Pointer provider = p_sys->provider_;
> +
> + item->type() == IItem::FileType::Directory ? i_type = ITEM_TYPE_DIRECTORY : i_type = ITEM_TYPE_FILE;
There is absolutely no need to separate declarations from the
initialization of said variable, and it would be far more appropriate
to declare the variable, `i_type` with the `const`-qualifier.
> + if (i_type == ITEM_TYPE_FILE)
> + {
> + item = provider->getItemDataAsync(item->id())->result();
> + return access_fsdir_additem(p_fsdir, item->url().c_str(), item->filename().c_str(), i_type, ITEM_NET);
> + }
> + else
> + {
> + std::string url = p_access->psz_url + item->filename() + "/";
> + return access_fsdir_additem(p_fsdir, url.c_str(), item->filename().c_str(), i_type, ITEM_NET);
Both of the above *return-statements* are pretty much equivalent,
introducing a variable to hold the second argument, and then narrowing
the function to have a single *point-of-return* will be cleaner than
the way it is currently written.
> + }
The lines in this function are very long, please try to keep them
shorter than `80` columns.
> +}
> +
> +static int readDir(stream_t *p_access, input_item_node_t *p_node)
> +{
> + access_sys_t *p_sys = (access_sys_t*)p_access->p_sys;
> + struct access_fsdir fsdir;
> +
> +
> + p_sys->list_directory_request_ = p_sys->provider_->listDirectoryAsync(p_sys->current_item_);
> + p_sys->directory_list_ = p_sys->list_directory_request_->result();
> + access_fsdir_init(&fsdir, p_access, p_node);
> +
> + auto finish = [&](int error)
> + {
> + access_fsdir_finish(&fsdir, error);
> + return error;
> + };
> +
> + for (auto &i : p_sys->directory_list_)
> + if (add_item(&fsdir, p_access, i))
> + return finish(VLC_EGENERIC);
> + return finish(VLC_SUCCESS);
> +}
> +
> +std::vector<std::string> parseUrl(std::string url)
> +{
> + std::vector<std::string> v;
> +
> + url = url.substr(15);
This could sure use a comment, as it is not immediate obvious that the
now magic-constant `15` is the length of `cloudstorage://`.
> + for (size_t i = 0; i < url.size(); ++i)
> + {
> + if (url[i] == '/')
> + {
> + v.push_back(url.substr(0, i));
> + url = url.substr(i + 1);
> + }
> + }
The above can be written as the below (untested, so please excuse any
potential typos):
std::vector<std::string> parts;
std::istringstream iss( url );
for( std::string s1; std::getline( iss, s1, '/' ); )
parts.push_back( s1 );
Your implementation also looks wrong, as you do not "reset" `i` even
though you are shrinking the `url`. If you are only interested in the
first `/`, please use `std::string::find`.
> + return v;
> +}
> +
> +static int getDir(stream_t *p_access, input_item_node_t *p_node)
> +{
> + access_sys_t *p_sys = (access_sys_t*)p_access->p_sys;
> +
> + if (strcmp(p_access->psz_url, "cloudstorage://") == 0)
> + readDir(p_access, p_node);
> + else
> + {
> + p_sys->directory_stack_ = parseUrl(p_access->psz_url);
> + for (auto &name : p_sys->directory_stack_)
> + {
> + p_sys->list_directory_request_ = p_sys->provider_->listDirectoryAsync(p_sys->current_item_);
> + p_sys->directory_list_ = p_sys->list_directory_request_->result();
The intermediate copy to `p_sys->directory_list_`, and the objects
inside the container, seems unnecessary; why not use
`p_sys->list_directory_request_->result()` directly in the
*for-statement*?
> + for (auto &item : p_sys->directory_list_)
> + if (item->filename() == name)
> + {
> + p_sys->current_item_ = item;
> + break;
> + }
The above can be written using `std::find_if` and an appropriate
*lambda*.
> + }
> + readDir(p_access, p_node);
> + }
> + return VLC_SUCCESS;
> +}
> +
> +static int Open(vlc_object_t *p_this)
This function, `Open`, seems to lack proper probing to make sure that
it is definitely not used when it should not; or am I missing
something?
If I am indeed missing something, me asking might be a sign that the
implementation needs a little cleaning, and/or that it lacks proper
documentation to make it easier to understand.
> +{
> + access_t *p_access = (access_t*)p_this;
> + access_sys_t *p_sys;
The above indentation looks weird, and while speaking about that
please do not use tabs for indentation (this applies to the entire
file as there are tabs all over the place).
> +
> + p_access->p_sys = p_sys = new access_sys_t(p_this);
The above needs `new (std::nothrow)`, as `new T` will throw an
exception of the allocation fails, rendering the if-statement that
follows redundant (as it will never be true).
> + if (p_sys == nullptr)
> + return VLC_ENOMEM;
> + p_access->pf_control = Control;
> + p_access->pf_readdir = getDir;
> + if (p_access->pf_control == nullptr
> + || p_access->pf_readdir == nullptr
The above checks related to `p_access->pf_control` and
`p_access->pf_readdir` is redundant; there is no way that these two
*data-members* can compare equal to `nullptr`.
> + || p_sys->p_keystore_ == nullptr)
This check should be done inside the constructor of `access_sys_t`,
and if it fails to create the keystore it should throw an exception.
> + goto error;
> + return VLC_SUCCESS;
> +
> + error:
> + Close(p_this);
> + return VLC_EGENERIC;
> +}
> +
> +static void Close(vlc_object_t *p_this)
> +{
> + access_t *p_access = (access_t*)p_this;
> + access_sys_t *p_sys = (access_sys_t*)p_access->p_sys;
> +
> +std::cout << "Close" << '\n';
> + delete(p_sys);
`delete` is not a function, please do not wrap the operand in
parenthesis... `delete p_sys` is certainly enough.
> + return;
> +}
> diff --git a/src/libvlccore.sym b/src/libvlccore.sym
> index 4cb7a22..25d01df 100644
> --- a/src/libvlccore.sym
> +++ b/src/libvlccore.sym
> @@ -591,6 +591,7 @@ vlc_ngettext
> vlc_iconv
> vlc_iconv_close
> vlc_iconv_open
> +get_memory_keystore
Remi's comment is related to the above, exported symbols shall have an
appropriate prefix.. as you can see, the other keystore related
functions start with `vlc_` (as should be the case for all newly
introduced vlc-exported names).
> vlc_keystore_create
> vlc_keystore_release
> vlc_keystore_find
> diff --git a/src/misc/keystore.c b/src/misc/keystore.c
> index 02537f0..c6096cf 100644
> --- a/src/misc/keystore.c
> +++ b/src/misc/keystore.c
> @@ -101,7 +101,7 @@ vlc_keystore_store(vlc_keystore *p_keystore,
> }
>
> unsigned int
> -vlc_keystore_find(vlc_keystore *p_keystore,
> +vlc_keystore_find(vlc_keystore *p_keystore,
> const char * const ppsz_values[KEY_MAX],
> vlc_keystore_entry **pp_entries)
> {
> @@ -148,7 +148,7 @@ libvlc_InternalKeystoreClean(libvlc_int_t *p_libvlc)
> }
> }
>
> -static vlc_keystore *
> +vlc_keystore *
> get_memory_keystore(vlc_object_t *p_obj)
> {
> return libvlc_priv(p_obj->obj.libvlc)->p_memory_keystore;
> --
> 2.7.4
>
Congratulations, on what afaik, your first submission to `vlc-devel`;
do not be scared by the feedback, and keep up the work!
Best Regards,\
Filip
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170424/4f023dc5/attachment.html>
More information about the vlc-devel
mailing list