Hi, Thanks for the review. Responses inline.
On 02-08-18 11:10, Antonio Quartulli wrote:
> On 26/07/18 00:08, Steffan Karger wrote:
>> diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
>> index 0972139..20e2b9c 100644
>> --- a/src/openvpn/buffer.c
>> +++ b/src/openvpn/buffer.c
>> @@ -343,16 +343,33 @@ convert_to_one_line(struct buffer *buf)
>> }
>> }
>>
>> -/* NOTE: requires that string be null terminated */
>> -void
>> -buf_write_string_file(const struct buffer *buf, const char *filename, int
>> fd)
>> +bool
>> +buffer_write_file(const char *filename, const struct buffer *buf)
>
> Since you are removing the old comments from both function declaration
> and definition, why not adding some little doxygen style doc?
In the .h, I added the following doxygen:
+/** Write buffer contents to file */
+bool buffer_write_file(const char *filename, const struct buffer *buf);
I'll expand it to explicitly state the return values, but I don't think
there's much more to say.
> I see you are also changing the way this method works (now it takes only
> the filename instead of a fd), but given that we currently use it only
> once in our code (for now), I think this change is ok and actually makes
> the logic more self-contained (no need for the caller to deal with the
> fd at all).
Yeah, that's the idea :) I was duplicating the surrounding code and
decided I didn't like that.
>> {
>> - const int len = strlen((char *) BPTR(buf));
>> - const int size = write(fd, BPTR(buf), len);
>> - if (size != len)
>> + bool ret = false;
>> + int fd = platform_open(filename, O_CREAT | O_TRUNC | O_WRONLY,
>> + S_IRUSR | S_IWUSR);
>> + if (fd == -1)
>> + {
>> + msg(M_ERRNO, "Cannot open file '%s' for write", filename);
>> + goto cleanup;
>> + }
>> +
>> + const int size = write(fd, BPTR(buf), BLEN(buf));
>> + if (size != BLEN(buf))
>> {
>> - msg(M_ERR, "Write error on file '%s'", filename);
>> + msg(M_ERRNO, "Write error on file '%s'", filename);
>> + goto cleanup;
>> + }
>> +
>> + ret = true;
>> +cleanup:
>> + if (close(fd))
>
> nitpikcing: I'd dare to say that in the rest of the code we prefer to
> use "func() < 0" as check, to make it more explicit that we are looking
> for errors (my personal feeling is that when i see something like your
> expression I always need to double check "does this function return
> something positive too? what in case of errors?").
>
> On top of that consistency is always good :-)
Ok, will change.
>> + {
>> + msg(M_ERRNO, "Close error on file %s", filename);
>> + ret = false;
>> }
>> + return ret;
>> }
>>
>> /*
>> diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h
>> index 6b6025e..180dd56 100644
>> --- a/src/openvpn/buffer.h
>> +++ b/src/openvpn/buffer.h
>> @@ -469,11 +469,8 @@ const char *skip_leading_whitespace(const char *str);
>>
>> void string_null_terminate(char *str, int len, int capacity);
>>
>> -/*
>> - * Write string in buf to file descriptor fd.
>> - * NOTE: requires that string be null terminated.
>> - */
>> -void buf_write_string_file(const struct buffer *buf, const char *filename,
>> int fd);
>> +/** Write buffer contents to file */
>> +bool buffer_write_file(const char *filename, const struct buffer *buf);
>>
>> /*
>> * write a string to the end of a buffer that was
>> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
>> index 5381ef2..35bd243 100644
>> --- a/src/openvpn/crypto.c
>> +++ b/src/openvpn/crypto.c
>> @@ -1427,27 +1427,19 @@ write_key_file(const int nkeys, const char *filename)
>> {
>> struct gc_arena gc = gc_new();
>>
>> - int fd, i;
>> - int nbits = 0;
>> + int nbits = nkeys * sizeof(struct key) * 8;
>>
>> /* must be large enough to hold full key file */
>> struct buffer out = alloc_buf_gc(2048, &gc);
>> - struct buffer nbits_head_text = alloc_buf_gc(128, &gc);
>>
>> /* how to format the ascii file representation of key */
>> const int bytes_per_line = 16;
>>
>> - /* open key file */
>> - fd = platform_open(filename, O_CREAT | O_TRUNC | O_WRONLY, S_IRUSR |
>> S_IWUSR);
>> -
>> - if (fd == -1)
>> - {
>> - msg(M_ERR, "Cannot open shared secret file '%s' for write",
>> filename);
>> - }
>> -
>> + /* write header */
>> + buf_printf(&out, "#\n# %d bit OpenVPN static key\n#\n", nbits);
>> buf_printf(&out, "%s\n", static_key_head);
>>
>> - for (i = 0; i < nkeys; ++i)
>> + for (int i = 0; i < nkeys; ++i)
>> {
>> struct key key;
>> char *fmt;
>> @@ -1463,9 +1455,6 @@ write_key_file(const int nkeys, const char *filename)
>> "\n",
>> &gc);
>>
>> - /* increment random bits counter */
>> - nbits += sizeof(key) * 8;
>> -
>> /* write to holding buffer */
>> buf_printf(&out, "%s\n", fmt);
>>
>> @@ -1476,17 +1465,8 @@ write_key_file(const int nkeys, const char *filename)
>>
>> buf_printf(&out, "%s\n", static_key_foot);
>>
>> - /* write number of bits */
>> - buf_printf(&nbits_head_text, "#\n# %d bit OpenVPN static key\n#\n",
>> nbits);
>> - buf_write_string_file(&nbits_head_text, filename, fd);
>> -
>> /* write key file, now formatted in out, to file */
>> - buf_write_string_file(&out, filename, fd);
>> -
>> - if (close(fd))
>> - {
>> - msg(M_ERR, "Close error on shared secret file %s", filename);
>> - }
>> + buffer_write_file(filename, &out);
>>
>> /* zero memory which held file content (memory will be freed by GC) */
>> buf_clear(&out);
>>
>
> I like this change especially because the writing function now returns a
> boolean reporting its success or failure. How about adding some more
> checks in the calling chain so that such status can be used?
>
> As of now, if the writing function fails, openvpn will print an error
> message, but won't really return "a failure" (as in returning non-zero).
> What do you think?
Absolutely right, will improve error handling and make openvpn return
non-zero.
-Steffan
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
