Re: [boinc_dev] [BOINC/boinc] 341001: lib: remove safe_copy(); not used, generated compi...

[Eric Korpela]

On an unrelated note, Please lets use autoconf like macros set up in an
appropriate config file rather than compiler or OS checking in the source
file, not compiler version checking in the source files.  WIN32 != MSVC.
We should for all contingencies.  IOW lets not assume every compiler but
visual C has strdup and only visual C has _strdup, and that every machine
has one or the other.  The logic for function choice can be complicated
and  should not be replicated in multiple source files unless absolutely
necessary.  Best if it's in two places and only two places...  the
configure script and boinc_win.h.  Otherwise our code gets littered with
"#if defined(_MSC_VER) || (defined(__MINGW32__) && MINGW32_PREREQ(3,11)) ||
(defined(__GNUC__) && (GNUC < 4))"

in boinc_win.h there should be
#if defined(_MSC_VER)  // better yet would be _MSC_VER_FULL >= version_num
#undef HAVE_STRDUP
#define HAVE__STRDUP 1
#endif

On every non-MSVC system, in config.h, configure will define HAVE_STRDUP
and HAVE__STRDUP appropriately.  Then safe_copy becomes

char *safe_copy(const char *cstr) {
#if defined(HAVE_STRDUP)
  return strdup(cstr);
#elif defined(HAVE__STRDUP)
  return _strdup(cstr);
#else
  char *s=malloc(strlen(cstr)+1);
  if (s) strcpy(s,cstr);
  return s;
#endif
}

Or we could code missing standard functions like strdup into std_fixes.h,
although that was originally workarounds for old g++ and MSVC bugs that are
probably no longer necessary.


On Wed, Jan 4, 2017 at 11:37 AM, Christian Beer 
wrote:

> On 04.01.2017 16:36, Juha Sointusalo wrote:
> > On 4 January 2017 at 02:24, Artem Vorotnikov  > > wrote:
> >
> > For the record, strdup() is *not* standard, it is a GNU extension and
> > not part of ANSI/ISO C.
> >
> >
> > It is in POSIX. MSVC knows it as _strdup() .
>
> It would still work by doing something like this:
>
> safe_copy(cstr){
> #ifdef MSVC
> return _strdup(cstr);
> #else
> return strdup(cstr);
> #endif
> }
>
> which would encapsulate the platform specific implementation nicely. But
> it would still allocate memory. But that was not my point in the beginning.
>
> I accept all reasons given in this email thread about why this function
> is problematic and maybe should be discussed. The reason I'm not willing
> to accept is the one given in the commit message. Just because something
> is producing warnings on one platform is not justification enough to
> remove it. That was the point I wanted to bring across.
>
> Regards
> Christian
>
> ___
> boinc_dev mailing list
> boinc_dev@ssl.berkeley.edu
> http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
> To unsubscribe, visit the above URL and
> (near bottom of page) enter your email address.
>



-- 
Eric Korpela
korp...@ssl.berkeley.edu
AST:7731^29u18e3
___
boinc_dev mailing list
boinc_dev@ssl.berkeley.edu
http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
To unsubscribe, visit the above URL and
(near bottom of page) enter your email address.

Re: [boinc_dev] [BOINC/boinc] 341001: lib: remove safe_copy(); not used, generated compi...

[Juha Sointusalo]

On 4 January 2017 at 02:24, Artem Vorotnikov  wrote:

> For the record, strdup() is *not* standard, it is a GNU extension and
> not part of ANSI/ISO C.
>

It is in POSIX. MSVC knows it as _strdup() .

-Juha
___
boinc_dev mailing list
boinc_dev@ssl.berkeley.edu
http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
To unsubscribe, visit the above URL and
(near bottom of page) enter your email address.

Re: [boinc_dev] [BOINC/boinc] 341001: lib: remove safe_copy(); not used, generated compi...

[Juha Sointusalo]

On 3 January 2017 at 17:13, Christian Beer 
wrote:

> So the safe_copy()
> function serves two purposes. It copies a string into a new char array
> to make it modifiable and usable outside of the string context.
>

Putting aside the bigger issues for a moment. The next time you need a copy
of a C string just use strdup(). It's standard and everybody knows it
allocates memory that needs to be free()d.

-Juha
___
boinc_dev mailing list
boinc_dev@ssl.berkeley.edu
http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
To unsubscribe, visit the above URL and
(near bottom of page) enter your email address.

Re: [boinc_dev] [BOINC/boinc] 341001: lib: remove safe_copy(); not used, generated compi...

[Christian Beer]

Is this an official BOINC policy? I can't find it on
http://boinc.berkeley.edu/trac/wiki/CodingStyle

Why does it need to be stated in the function name? Is there a common
naming scheme for this kind of documentation? Does this apply to other
functions too?

Regards
Christian

On 03.01.2017 17:17, David Anderson wrote:
> The policy in BOINC is to minimize dynamic allocation and thus avoid
> the complexity of pointer ownership.
> We avoid functions that return pointers to allocated memory.
>
> Exceptions to this should be clearly indicated in the function name,
> e.g. read_file_malloc().
>
> On 1/3/2017 7:13 AM, Christian Beer wrote:
>> On 03.01.2017 15:29, McLeod, John wrote:
>>> What about the std::string function c_str?  It returns a
>>> non-modifiable pointer to the null terminated data in the string?
>> This function (std::string::c_str) provides a non-modifiable pointer and
>> the pointer is only valid as long as the string exists. So you can't use
>> the pointer outside of the context of the string. So the safe_copy()
>> function serves two purposes. It copies a string into a new char array
>> to make it modifiable and usable outside of the string context.
>>
>> Another solution would be to only use strings and use c_str() locally
>> when it is needed and not pass around pointers to char arrays.
>>
>> Regards
>> Christian
>>
>> ___
>> boinc_dev mailing list
>> boinc_dev@ssl.berkeley.edu
>> http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
>> To unsubscribe, visit the above URL and
>> (near bottom of page) enter your email address.
>
> ___
> boinc_dev mailing list
> boinc_dev@ssl.berkeley.edu
> http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
> To unsubscribe, visit the above URL and
> (near bottom of page) enter your email address.


___
boinc_dev mailing list
boinc_dev@ssl.berkeley.edu
http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
To unsubscribe, visit the above URL and
(near bottom of page) enter your email address.

Re: [boinc_dev] [BOINC/boinc] 341001: lib: remove safe_copy(); not used, generated compi...

[Christian Beer]

The scheduler can also be used as an fcgi executable in which case it
runs indefinitely like the file_upload_handler which would expose the
memory leak.

Regards
Christian

On 03.01.2017 17:18, David Anderson wrote:
> It's OK in this case because it's in the scheduler, which runs a
> limited number of times then exits.
> It's not OK in the client.
>
> On 1/3/2017 7:34 AM, Christian Beer wrote:
>> As for memory leaks occurring in BOINC I'd like to quote
>> sched/sched_version.cpp line 515:
>>> // This is a memory leak, but that's OK
>>
>
> ___
> boinc_dev mailing list
> boinc_dev@ssl.berkeley.edu
> http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
> To unsubscribe, visit the above URL and
> (near bottom of page) enter your email address.


___
boinc_dev mailing list
boinc_dev@ssl.berkeley.edu
http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
To unsubscribe, visit the above URL and
(near bottom of page) enter your email address.

Re: [boinc_dev] [BOINC/boinc] 341001: lib: remove safe_copy(); not used, generated compi...

[David Anderson]

It's OK in this case because it's in the scheduler, which runs a limited number of 
times then exits.

It's not OK in the client.

On 1/3/2017 7:34 AM, Christian Beer wrote:

As for memory leaks occurring in BOINC I'd like to quote
sched/sched_version.cpp line 515:

// This is a memory leak, but that's OK




___
boinc_dev mailing list
boinc_dev@ssl.berkeley.edu
http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
To unsubscribe, visit the above URL and
(near bottom of page) enter your email address.

Re: [boinc_dev] [BOINC/boinc] 341001: lib: remove safe_copy(); not used, generated compi...

[David Anderson]

The policy in BOINC is to minimize dynamic allocation and thus avoid
the complexity of pointer ownership.
We avoid functions that return pointers to allocated memory.

Exceptions to this should be clearly indicated in the function name,
e.g. read_file_malloc().

On 1/3/2017 7:13 AM, Christian Beer wrote:

On 03.01.2017 15:29, McLeod, John wrote:

What about the std::string function c_str?  It returns a non-modifiable pointer 
to the null terminated data in the string?

This function (std::string::c_str) provides a non-modifiable pointer and
the pointer is only valid as long as the string exists. So you can't use
the pointer outside of the context of the string. So the safe_copy()
function serves two purposes. It copies a string into a new char array
to make it modifiable and usable outside of the string context.

Another solution would be to only use strings and use c_str() locally
when it is needed and not pass around pointers to char arrays.

Regards
Christian

___
boinc_dev mailing list
boinc_dev@ssl.berkeley.edu
http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
To unsubscribe, visit the above URL and
(near bottom of page) enter your email address.


___
boinc_dev mailing list
boinc_dev@ssl.berkeley.edu
http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
To unsubscribe, visit the above URL and
(near bottom of page) enter your email address.

Re: [boinc_dev] [BOINC/boinc] 341001: lib: remove safe_copy(); not used, generated compi...

[Christian Beer]

I agree. But this would also be the more expensive solution (in terms of
man-hours). And also given the fact that we don't have a test framework
to make sure that switching to std::string does not break anything adds
a lot of risk. Whereas my little function has a defined behavior and a
small risk.

As for memory leaks occurring in BOINC I'd like to quote
sched/sched_version.cpp line 515:
> // This is a memory leak, but that's OK

Regards
Christian

On 03.01.2017 16:15, McLeod, John wrote:
> The second would be far better.  It would tend to stop memory leaks from 
> occurring.
>
> -Original Message-
> From: Christian Beer [mailto:christian.b...@aei.mpg.de] 
> Sent: Tuesday, January 3, 2017 10:13 AM
> To: McLeod, John <john.mcl...@sap.com>; boinc_dev@ssl.berkeley.edu
> Subject: Re: [boinc_dev] [BOINC/boinc] 341001: lib: remove safe_copy(); not 
> used, generated compi...
>
> On 03.01.2017 15:29, McLeod, John wrote:
>> What about the std::string function c_str?  It returns a non-modifiable 
>> pointer to the null terminated data in the string?
> This function (std::string::c_str) provides a non-modifiable pointer and
> the pointer is only valid as long as the string exists. So you can't use
> the pointer outside of the context of the string. So the safe_copy()
> function serves two purposes. It copies a string into a new char array
> to make it modifiable and usable outside of the string context.
>
> Another solution would be to only use strings and use c_str() locally
> when it is needed and not pass around pointers to char arrays.
>
> Regards
> Christian
>

___
boinc_dev mailing list
boinc_dev@ssl.berkeley.edu
http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
To unsubscribe, visit the above URL and
(near bottom of page) enter your email address.

Re: [boinc_dev] [BOINC/boinc] 341001: lib: remove safe_copy(); not used, generated compi...

[McLeod, John]

The second would be far better.  It would tend to stop memory leaks from 
occurring.

-Original Message-
From: Christian Beer [mailto:christian.b...@aei.mpg.de] 
Sent: Tuesday, January 3, 2017 10:13 AM
To: McLeod, John <john.mcl...@sap.com>; boinc_dev@ssl.berkeley.edu
Subject: Re: [boinc_dev] [BOINC/boinc] 341001: lib: remove safe_copy(); not 
used, generated compi...

On 03.01.2017 15:29, McLeod, John wrote:
> What about the std::string function c_str?  It returns a non-modifiable 
> pointer to the null terminated data in the string?

This function (std::string::c_str) provides a non-modifiable pointer and
the pointer is only valid as long as the string exists. So you can't use
the pointer outside of the context of the string. So the safe_copy()
function serves two purposes. It copies a string into a new char array
to make it modifiable and usable outside of the string context.

Another solution would be to only use strings and use c_str() locally
when it is needed and not pass around pointers to char arrays.

Regards
Christian

___
boinc_dev mailing list
boinc_dev@ssl.berkeley.edu
http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
To unsubscribe, visit the above URL and
(near bottom of page) enter your email address.

Re: [boinc_dev] [BOINC/boinc] 341001: lib: remove safe_copy(); not used, generated compi...

[Christian Beer]

On 03.01.2017 15:29, McLeod, John wrote:
> What about the std::string function c_str?  It returns a non-modifiable 
> pointer to the null terminated data in the string?

This function (std::string::c_str) provides a non-modifiable pointer and
the pointer is only valid as long as the string exists. So you can't use
the pointer outside of the context of the string. So the safe_copy()
function serves two purposes. It copies a string into a new char array
to make it modifiable and usable outside of the string context.

Another solution would be to only use strings and use c_str() locally
when it is needed and not pass around pointers to char arrays.

Regards
Christian

___
boinc_dev mailing list
boinc_dev@ssl.berkeley.edu
http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
To unsubscribe, visit the above URL and
(near bottom of page) enter your email address.

Re: [boinc_dev] [BOINC/boinc] 341001: lib: remove safe_copy(); not used, generated compi...

[McLeod, John]

What about the std::string function c_str?  It returns a non-modifiable pointer 
to the null terminated data in the string?

-Original Message-
From: boinc_dev [mailto:boinc_dev-boun...@ssl.berkeley.edu] On Behalf Of 
Christian Beer
Sent: Tuesday, December 20, 2016 7:22 AM
To: boinc_dev@ssl.berkeley.edu
Subject: Re: [boinc_dev] [BOINC/boinc] 341001: lib: remove safe_copy(); not 
used, generated compi...

I don't care about the function. I'll just put it back in when I need it
again. What I'm more offended by is the reason that is stated in the
commit message. In my opinion that is not a valid reason to remove
contributed code without discussion or the chance for the contributor to
explain purpose or rectify errors.

- This function represents a nice way to convert std::string to char*
and may become handy in the future. I put it in BOINC because it is not
project-specific and may be something projects find helpful to be in the
BOINC library.
- Indicating implementation details in function names is not useful in
this context in my opinion. From the name alone I wouldn't think that I
need to free the memory myself. Such kind of post conditions are usually
expressed in a descriptive comment above the function. Usually in a
format that can be understood by a documentation tool like doxygen where
the post condition would then be part of the developer documentation
(the place where I would look for such post conditions).
- With a proper post condition that states "You are the owner of the
allocated memory of this function, clean this up after you used it" it
should be clear who the owner is.

Regards
Christian

On 19.12.2016 22:02, David Anderson wrote:
> I'll put it back if you feel that strongly.
> I removed it because:
> - it's not used anywhere (if needed for project-specific code, can
> define it there)
> - the name should be something like safe_copy_alloc() to indicate that
> it allocates memory
> - I try to avoid memory allocation in BOINC because pointer ownership
> is gnarly.
> .
> - David
>
> On 12/19/2016 5:18 AM, Christian Beer wrote:
>> Hi,
>>
>> am I the only one who has a problem with this commit?
>>
>> The BOINC Governance document states that actions of Committers are
>> reviewed after they committed something. Here we are. I want to complain
>> about this commit by a Committer of the BOJNC project.
>>
>> I added this function for a reason and yes it was intended to be used in
>> the server which means Linux only. So if it produces compile errors on
>> Windows we do have better options than removing it from the library. The
>> fact that it is currently not used in the BOINC code should not be the
>> sole reason to throw it away. hat if a project uses this function in
>> project specific daemons?
>>
>> Regards
>> Christian
>>
>> On 19.12.2016 10:17, GitHub wrote:
>>> Branch: refs/heads/master
>>>Home:   https://github.com/BOINC/boinc
>>>Commit: 3410015b00f13f53e8284e668b503b31c20814e4
>>>   
>>> https://github.com/BOINC/boinc/commit/3410015b00f13f53e8284e668b503b31c20814e4
>>>Author: David Anderson <da...@ssl.berkeley.edu>
>>>Date:   2016-12-19 (Mon, 19 Dec 2016)
>>>
>>>Changed paths:
>>>  M lib/str_util.cpp
>>>  M lib/str_util.h
>>>
>>>Log Message:
>>>---
>>>lib: remove safe_copy(); not used, generated compile warnings on Win
>>
>> ___
>> boinc_dev mailing list
>> boinc_dev@ssl.berkeley.edu
>> http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
>> To unsubscribe, visit the above URL and
>> (near bottom of page) enter your email address.
>
> ___
> boinc_dev mailing list
> boinc_dev@ssl.berkeley.edu
> http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
> To unsubscribe, visit the above URL and
> (near bottom of page) enter your email address.


___
boinc_dev mailing list
boinc_dev@ssl.berkeley.edu
http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
To unsubscribe, visit the above URL and
(near bottom of page) enter your email address.
___
boinc_dev mailing list
boinc_dev@ssl.berkeley.edu
http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
To unsubscribe, visit the above URL and
(near bottom of page) enter your email address.

Re: [boinc_dev] [BOINC/boinc] 341001: lib: remove safe_copy(); not used, generated compi...

[David Anderson]

I'll put it back if you feel that strongly.
I removed it because:
- it's not used anywhere (if needed for project-specific code, can define it 
there)
- the name should be something like safe_copy_alloc() to indicate that it allocates 
memory

- I try to avoid memory allocation in BOINC because pointer ownership is gnarly.
.
- David

On 12/19/2016 5:18 AM, Christian Beer wrote:

Hi,

am I the only one who has a problem with this commit?

The BOINC Governance document states that actions of Committers are
reviewed after they committed something. Here we are. I want to complain
about this commit by a Committer of the BOJNC project.

I added this function for a reason and yes it was intended to be used in
the server which means Linux only. So if it produces compile errors on
Windows we do have better options than removing it from the library. The
fact that it is currently not used in the BOINC code should not be the
sole reason to throw it away. hat if a project uses this function in
project specific daemons?

Regards
Christian

On 19.12.2016 10:17, GitHub wrote:

Branch: refs/heads/master
   Home:   https://github.com/BOINC/boinc
   Commit: 3410015b00f13f53e8284e668b503b31c20814e4
   
https://github.com/BOINC/boinc/commit/3410015b00f13f53e8284e668b503b31c20814e4
   Author: David Anderson 
   Date:   2016-12-19 (Mon, 19 Dec 2016)

   Changed paths:
 M lib/str_util.cpp
 M lib/str_util.h

   Log Message:
   ---
   lib: remove safe_copy(); not used, generated compile warnings on Win


___
boinc_dev mailing list
boinc_dev@ssl.berkeley.edu
http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
To unsubscribe, visit the above URL and
(near bottom of page) enter your email address.


___
boinc_dev mailing list
boinc_dev@ssl.berkeley.edu
http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
To unsubscribe, visit the above URL and
(near bottom of page) enter your email address.

Re: [boinc_dev] [BOINC/boinc] 341001: lib: remove safe_copy(); not used, generated compi...

[Christian Beer]

Hi,

am I the only one who has a problem with this commit?

The BOINC Governance document states that actions of Committers are
reviewed after they committed something. Here we are. I want to complain
about this commit by a Committer of the BOJNC project.

I added this function for a reason and yes it was intended to be used in
the server which means Linux only. So if it produces compile errors on
Windows we do have better options than removing it from the library. The
fact that it is currently not used in the BOINC code should not be the
sole reason to throw it away. hat if a project uses this function in
project specific daemons?

Regards
Christian

On 19.12.2016 10:17, GitHub wrote:
> Branch: refs/heads/master
>   Home:   https://github.com/BOINC/boinc
>   Commit: 3410015b00f13f53e8284e668b503b31c20814e4
>   
> https://github.com/BOINC/boinc/commit/3410015b00f13f53e8284e668b503b31c20814e4
>   Author: David Anderson 
>   Date:   2016-12-19 (Mon, 19 Dec 2016)
>
>   Changed paths:
> M lib/str_util.cpp
> M lib/str_util.h
>
>   Log Message:
>   ---
>   lib: remove safe_copy(); not used, generated compile warnings on Win


___
boinc_dev mailing list
boinc_dev@ssl.berkeley.edu
http://lists.ssl.berkeley.edu/mailman/listinfo/boinc_dev
To unsubscribe, visit the above URL and
(near bottom of page) enter your email address.