[vlc-devel] [PATCH] Basic MTP Support

[Rafaël Carré]

2008/3/26, Fabio Ritrovato <exsephiroth87 at gmail.com>:
> Well, funman said that editing configure.ac was not necessarily for my
> qualification...

but it is better .. ;)
At a start you can just copy the detection from another module, and
change it to your needs.
Open Source is great because you can copy/paste (but don't paste bugs !)

Now about the patch:

+ * Authors: Fabio Ritrovato <exsephiroth87 at gmail.com>
+ *         Christophe Massiot <massiot at via.ecp.fr>
+ *          Rémi Denis-Courmont <rem # videolan # org>
Did they help you to code ?

+#include <getopt.h>
You don't use that one

+       char to_path[256];
+       to_path[0]='\0';
+       strcat( to_path, getenv( "HOME" ) );
+       strcat( to_path, "/.vlc/" );
+       strcat( to_path, p_access->psz_path );
+       msg_Dbg( p_access, "About to write %s...", to_path );
There is an obvious buffer overflow.

I'm not sure to understand why you have to write something in ~/.vlc ?

+  current = malloc (strlen(parent) + strlen(folder->name) + 2);
You don't check if malloc() succeeded or not

Are you sure the access module has not unneeded parts from file.c ?
I'll jump to services discovery:

+ * Copyright (c) 2006-2007 Rafaël Carré
No :p

+/*#ifdef HAVE_UNISTD_H
+#    include <unistd.h>
+#endif*/
Leftover debug code ?

+    services_discovery_sys_t *p_sys  = malloc(
+                                    sizeof( services_discovery_sys_t ) );
You don't check if malloc() failed

+           psz_name = LIBMTP_Get_Modelname( p_device );
+       msg_Info( p_sd, "%s", psz_name );
Is that function guaranteed to succeed ?
If not you should use a fallback like _("Unknown")

+       services_discovery_SetLocalizedName( p_sd, _(psz_name) );
_() returns the translated version of a string, however you can not
use it on an "user-provided" (in this case library provided) string,
since it is not known to translators.
Here it should be something like _("MTP devices")

+   else
+   {
+       msg_Info( p_sd, "No device found..." );
+       free( p_sys );
Too bad :( can't you detect devices addition ? For example I launch
VLC, and THEN I connect my mtp device
The same apply for Run() function

static void Close( vlc_object_t *p_this )
+{
+   services_discovery_t *p_sd = ( services_discovery_t* )p_this;
+
+   free( p_sd->p_sys );
+}
It seems strange to me, you don't have to clean anything allocated by libmtp ?

+   psz_path=malloc((strlen(p_track->filename)+7)*sizeof(char));
+   sprintf(psz_path, "mtp://%s", p_track->filename);
malloc() not checked

It seems the libmtp plays file mounted on a filesystem, I thought it
was not needed, was I wrong ?


Thanks for your work, I hope to see an updated patch soon ;)

-- 
Rafaël Carré