<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<meta http-equiv="Content-Style-Type" content="text/css" />
<meta name="generator" content="pandoc" />
<title></title>
<style type="text/css">code{white-space: pre;}</style>
</head>
<body>
<p>Hi William,</p>
<p>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.</p>
<p>On 2017-04-19 16:24, William Ung wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> ---
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_;</code></pre>
</blockquote>
<p>The above <em>data-members</em> seems to always be assigned before being read, is it required to keep the objects referred to by the <code>IItem::Pointer</code> alive between calls? If so; that should be documented.</p>
<p>If the above is not true, please replace the usage of this <em>data-member</em> with a variable at the places where it is currently used.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+ ICloudProvider::ListDirectoryRequest::Pointer list_directory_request_;
+
+ std::mutex stream_mutex_;
+ std::condition_variable stream_cv_;</code></pre>
</blockquote>
<p>These two data-members are not used anywhere in your implementation, what are there purpose? If there is a need for <em>thread-synchronization</em>, that should be fixed prior to submission.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+ access_sys_t(vlc_object_t *);
+};
+
+/*
+** Provider Callback
+*/
+
+class Callback : public ICloudProvider::ICallback {
+public:
+ Callback(access_sys_t *sys)
+ {
+ p_sys = sys;</code></pre>
</blockquote>
<p>Please use the <em>mem-initializer list</em> instead of plain assignment to the relevant <em>data-member</em>, it is cleaner and strictly speaking more correct.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + }
+
+ Status userConsentRequired(const ICloudProvider& provider) override
+ {
+ std::cout << "User ConsentRequired at : " << provider.authorizeLibraryUrl() << std::endl;
+ return Status::WaitForAuthorizationCode;</code></pre>
</blockquote>
<p>Diagnostics should not be printed to <code>std::cout</code>, nor any other standard output-streams. Please use <code>msg_{Dbg,Warn,Err}</code> as provided by VLC.</p>
<p>This comment applies to all occurances of usage of <code>std::cout</code>.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + }
+
+ 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());</code></pre>
</blockquote>
<p>I am aware of the fact that the documentation for <code>vlc_keystore_store</code> 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 <code>vlc_keystore_remove</code> prior to a call to <code>vlc_keystore_store</code>.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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";
+}</code></pre>
</blockquote>
<p>There’s no point in having a helper-function with <code>3</code> lines, that is only called once; please do manual inlining at the site of call.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+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;</code></pre>
</blockquote>
<p>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 <code>provider_->initialize</code>, it at least looks weird).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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';</code></pre>
</blockquote>
<p>This is most definitely a left-over from some lazy-man debugging, but please try to refrain from pushing patches to <code>vlc-devel</code> that include such.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + token_ = (char *)p_entries[0].p_secret;
+ }
+ provider_->initialize({token_, std::unique_ptr<Callback>(new Callback(this)), nullptr, nullptr, nullptr, {}});</code></pre>
</blockquote>
<p>Please remember that exceptions that propagate out from the <em>“c++ world”</em> will invoke <em>undefined-behavior</em>, as such you will need to handle cases where an exception may occur, and prevent them from “leaking” out into the surrounding C world.</p>
<p>Long story, short; you will need <em>try-catch</em> clauses in all functions that are called from the core of VLC (as these shall have C linkage, and as such are not <em>“exception friendly”</em>).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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 );
+ }</code></pre>
</blockquote>
<p><code>switch</code>-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.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+ 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;</code></pre>
</blockquote>
<p>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, <code>i_type</code> with the <code>const</code>-qualifier.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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);</code></pre>
</blockquote>
<p>Both of the above <em>return-statements</em> are pretty much equivalent, introducing a variable to hold the second argument, and then narrowing the function to have a single <em>point-of-return</em> will be cleaner than the way it is currently written.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + }</code></pre>
</blockquote>
<p>The lines in this function are very long, please try to keep them shorter than <code>80</code> columns.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +}</code></pre>
</blockquote>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+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);</code></pre>
</blockquote>
<p>This could sure use a comment, as it is not immediate obvious that the now magic-constant <code>15</code> is the length of <code>cloudstorage://</code>.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + for (size_t i = 0; i < url.size(); ++i)
+ {
+ if (url[i] == '/')
+ {
+ v.push_back(url.substr(0, i));
+ url = url.substr(i + 1);
+ }
+ }</code></pre>
</blockquote>
<p>The above can be written as the below (untested, so please excuse any potential typos):</p>
<pre><code>std::vector<std::string> parts;
std::istringstream iss( url );
for( std::string s1; std::getline( iss, s1, '/' ); )
parts.push_back( s1 );</code></pre>
<p>Your implementation also looks wrong, as you do not “reset” <code>i</code> even though you are shrinking the <code>url</code>. If you are only interested in the first <code>/</code>, please use <code>std::string::find</code>.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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();</code></pre>
</blockquote>
<p>The intermediate copy to <code>p_sys->directory_list_</code>, and the objects inside the container, seems unnecessary; why not use <code>p_sys->list_directory_request_->result()</code> directly in the <em>for-statement</em>?</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + for (auto &item : p_sys->directory_list_)
+ if (item->filename() == name)
+ {
+ p_sys->current_item_ = item;
+ break;
+ }</code></pre>
</blockquote>
<p>The above can be written using <code>std::find_if</code> and an appropriate <em>lambda</em>.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + }
+ readDir(p_access, p_node);
+ }
+ return VLC_SUCCESS;
+}
+
+static int Open(vlc_object_t *p_this)</code></pre>
</blockquote>
<p>This function, <code>Open</code>, seems to lack proper probing to make sure that it is definitely not used when it should not; or am I missing something?</p>
<p>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.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +{
+ access_t *p_access = (access_t*)p_this;
+ access_sys_t *p_sys;</code></pre>
</blockquote>
<p>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>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+ p_access->p_sys = p_sys = new access_sys_t(p_this);</code></pre>
</blockquote>
<p>The above needs <code>new (std::nothrow)</code>, as <code>new T</code> will throw an exception of the allocation fails, rendering the if-statement that follows redundant (as it will never be true).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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</code></pre>
</blockquote>
<p>The above checks related to <code>p_access->pf_control</code> and <code>p_access->pf_readdir</code> is redundant; there is no way that these two <em>data-members</em> can compare equal to <code>nullptr</code>.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + || p_sys->p_keystore_ == nullptr)</code></pre>
</blockquote>
<p>This check should be done inside the constructor of <code>access_sys_t</code>, and if it fails to create the keystore it should throw an exception.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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);</code></pre>
</blockquote>
<p><code>delete</code> is not a function, please do not wrap the operand in parenthesis… <code>delete p_sys</code> is certainly enough.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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</code></pre>
</blockquote>
<p>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 <code>vlc_</code> (as should be the case for all newly introduced vlc-exported names).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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
</code></pre>
</blockquote>
<p>Congratulations, on what afaik, your first submission to <code>vlc-devel</code>; do not be scared by the feedback, and keep up the work!</p>
<p>Best Regards,<br />
Filip</p>
</body>
</html>