After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 709795 - curlsftpsink - new libcurl-based sink element for SFTP
curlsftpsink - new libcurl-based sink element for SFTP
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-10-10 08:16 UTC by Sorin L.
Modified: 2013-11-01 16:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Implements a sink element which uploads data to a SFTP server (40.63 KB, patch)
2013-10-10 08:16 UTC, Sorin L.
needs-work Details | Review
...updates according to the first review... (40.73 KB, patch)
2013-10-11 13:45 UTC, Sorin L.
committed Details | Review

Description Sorin L. 2013-10-10 08:16:15 UTC
Created attachment 256880 [details] [review]
Implements a sink element which uploads data to a SFTP server

I extended the collection of curl-based sink elements with a new one supporting SFTP (SSH File Transfer Protocol).

The sink acts as a client and uploads data to a SFTP server.
Comment 1 Sebastian Dröge (slomo) 2013-10-10 12:40:17 UTC
Review of attachment 256880 [details] [review]:

::: ext/curl/Makefile.am
@@ +1,3 @@
 plugin_LTLIBRARIES = libgstcurl.la
 
+if USE_SSH2

You should probably add the SSH2 CFLAGS and LIBS to the CFLAGS and LIBADD variables too

::: ext/curl/gstcurlsftpsink.c
@@ +31,3 @@
+ * gst-launch filesrc location=/home/jdoe/some.file ! curlsftpsink  \
+ *     file-name=some.file.backup  \
+ *     user=john location=sftp://192.168.0.1/~/sftp_tests/  \

Why not combine location and filename?

::: ext/curl/gstcurlsshsink.c
@@ +141,3 @@
+
+  g_object_class_install_property (gobject_class, PROP_SSH_PVT_KEYFILE,
+      g_param_spec_string ("ssh-pvt-keyfile",

pvt -> priv?

@@ +201,3 @@
+
+  gst_element_get_state (GST_ELEMENT (sink), &cur_state, NULL, 0);
+  if (cur_state != GST_STATE_PLAYING && cur_state != GST_STATE_PAUSED) {

maybe inverse this and return, instead of indenting the complete block below

@@ +371,3 @@
+   * is also set! */
+  if ((curl_err = curl_easy_setopt (bcsink->curl, CURLOPT_SSH_KEYFUNCTION,
+              klass->curl_sshkey_cb)) != CURLE_OK) {

Why is the callback stored in the class?
Comment 2 Sorin L. 2013-10-11 13:43:35 UTC
Review of attachment 256880 [details] [review]:

I will upload a new patch updated according to your remarks. Hope I didn't missed anything.
Thanks for the quick feedback, btw.

::: ext/curl/gstcurlsftpsink.c
@@ +31,3 @@
+ * gst-launch filesrc location=/home/jdoe/some.file ! curlsftpsink  \
+ *     file-name=some.file.backup  \
+ *     user=john location=sftp://192.168.0.1/~/sftp_tests/  \

That was a design decision made earlier (see gstcurlbasesink.c) and, as I understand it, it gives us more flexibility having it this way.
The file name can change dynamically while the pipeline is PLAYING (for example it can get a timestamp appended as suffix) while the destination location remains the same. The effective upload is handled in a separate thread which gets notified of file name changes.
Comment 3 Sorin L. 2013-10-11 13:45:25 UTC
Created attachment 257002 [details] [review]
...updates according to the first review...
Comment 4 Sebastian Dröge (slomo) 2013-11-01 16:22:21 UTC
commit 15717842e426187635d934c16b1f2ddd6b96eb2a
Author: L. Sorin <sorin@axis.com>
Date:   Fri Oct 4 12:48:10 2013 +0200

    curl: curlsftpsink - new libcurl-based sink element for SFTP
    
    Note: SFTP = SSH File Transfer Protocol
    The sink acts as a client and uploads data to the SFTP server.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=709795