[Pkg-owncloud-maintainers] FTBS ocsync & qtkeychain

[Pino Toscano]

Hi,

In data giovedì 19 settembre 2013 12:16:49, Svante Signell ha scritto:
> The patch is attached. Additionally, the freebsd patch is modified to
> include errno.h for definition of ENODATA and EPIPE. The compiler
> complains that EPIPE is not defined otherwise. Maybe this file can be
> included unconditionally, without harm. I wonder how the error codes
> are defined for freebsd if errno.h is not included?

Reviews below.

> --- a/src/csync_misc.c  2013-09-04 11:15:42.000000000 +0200
> +++ b/src/csync_misc.c  2013-09-19 07:09:54.000000000 +0200
> @@ -88,20 +88,15 @@ char *csync_get_local_username(void) {
>  #endif /* NSS_BUFLEN_PASSWD */
>  
>  char *csync_get_user_home_dir(void) {
> -    char home[PATH_MAX] = {0};
> -    const char *envp;
> +    static const char *home = NULL;
>      struct passwd pwd;
>      struct passwd *pwdbuf;
>      char buf[NSS_BUFLEN_PASSWD];
>      int rc;
>  
> -    envp = getenv("HOME");
> -    if (envp != NULL) {
> -        snprintf(home, sizeof(home), "%s", envp);
> -        if (home[0] != '\0') {
> -            return c_strdup(home);
> -        }
> -    }
> +    home = getenv("HOME");
> +    if (home)
> +      return c_strdup(home);

Storing statically the result of getenv is wrong, since it points to the 
global environment array which can be reallocated anytime.
Also, no need to rename envp to home, makes the change bigger for no 
reason.
Furthermore, your changes are losing the check for an empty HOME.

A better change is the following, which is easier:

--- a/src/csync_misc.c
+++ b/src/csync_misc.c
@@ -88,7 +88,6 @@
 #endif /* NSS_BUFLEN_PASSWD */

 char *csync_get_user_home_dir(void) {
-    char home[PATH_MAX] = {0};
     const char *envp;
     struct passwd pwd;
     struct passwd *pwdbuf;
@@ -96,11 +95,8 @@
     int rc;

     envp = getenv("HOME");
-    if (envp != NULL) {
-        snprintf(home, sizeof(home), "%s", envp);
-        if (home[0] != '\0') {
-            return c_strdup(home);
-        }
+    if (envp != NULL && envp[0] != '\0') {
+        return c_strdup(envp);
     }

     /* Still nothing found, read the password file */

> --- a/src/std/c_private.h
> +++ b/src/std/c_private.h
> @@ -26,6 +26,10 @@
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  
> +#ifdef __GNU__
> +#include <errno.h>
> +#endif
> +

No need to misuse __GNU__ for this, errno.h is a standard and such it 
can be included anytime. In case it poses issues on Windows (sigh), you 
can include it in the else of the _WIN32 block (as people in -bsd@ 
suggested regarding fcntl.h).

>  #ifdef _WIN32
>  #include <windef.h>
>  #include <winbase.h>
> @@ -50,6 +54,11 @@
>  #define geteuid() 0
>  #endif
>  
> +#ifndef ENODATA
> +#define ENODATA EPIPE
> +#endif

This may be problematic. c_copy in src/std/c_file.c uses ENODATA as 
errno in a couple of error situations, but since c_copy is an exported 
symbol (although its header is not installed), users of this function 
would need to do this ENODATA→EPIPE mapping themselves too.

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.alioth.debian.org/pipermail/pkg-owncloud-maintainers/attachments/20130919/51cd1b36/attachment.sig>