Hi Artur,
This patch has whitespace damage. If you use Evolution to insert
the patch make sure you use the "Preformat" formatting option. A
better alternative is to use the "stg mail" command from Stacked GIT
which, among other things, will format the patch according to akpm's
"perfect patch" guidelines [1].
Some comments below:
> @@ -899,19 +900,12 @@ static int imsm_enumerate_ports(const char
> *hba_path, int port_count, int host_b
> }
>
> /* retrieve the scsi device type */
> - if (asprintf(&device, "/sys/dev/block/%d:%d/device/xxxxxxx", major,
> minor) < 0) {
> - if (verbose)
> - fprintf(stderr, Name ": failed to allocate 'device'\n");
> - err = 2;
> - break;
> - }
> - sprintf(device, "/sys/dev/block/%d:%d/device/type", major, minor);
> + str_fmt(device, "/sys/dev/block/%d:%d/device/type", major, minor);
Admittedly this change isn't needed because asprintf has guaranteed
that the buffer is large enough, but I can see why you made the change
if the goal is eradication of all sprintf calls.
> @@ -1425,6 +1428,36 @@ void append_metadata_update(struct supertype *st,
> void *buf, int len)
> }
> #endif /* MDASSEMBLE */
>
> +/* Copyright (C) 2009 Intel Corporation. All rights reserved.
> + *
It is not conventional to claim copyright on a per function basis.
> + * This function formats a string according to format pattern. The
> buffer is
> + * always null terminated even if source string does not fit in
> destination
> + * buffer. The function returns -1 in case of an error and this means
> + * either one of the input parameters is NULL or there's not enough
> space in
> + * destination buffer to fit even a single character. Otherwise the
> function
> + * returns the number of character put in the destination buffer.
> + */
> +int __str_fmt(char *buf, size_t buf_size, const char *fmt, ...)
> +{
> + va_list vl;
> +
> + if (((int)(--buf_size)) <= 0) {
This seems wrong. Why check buf_size? Just let the normal return
value from vnsprintf indicate if the buffer is too small. Also it
clips potentially valid sizes that appear negative when casting from
size_t to int.
> + errno = ENOBUFS;
> + return -1;
> + }
> + if ((buf == NULL) || (fmt == NULL)) {
> + errno = EINVAL;
> + return -1;
> + }
> + va_start(vl, fmt);
> + int result = vsnprintf(buf, buf_size, fmt, vl);
> + va_end(vl);
> + if ((result < 0) || (result >= buf_size)) {
> + buf[result = buf_size] = '\0';
Move that assignment to its own line. In general mdadm tends to
follow kernel coding standards, Neil, of course feel free to clarify.
> + }
> + return result;
> +}
> +
> #ifdef __TINYC__
> /* tinyc doesn't optimize this check in ioctl.h out ... */
> unsigned int __invalid_size_argument_for_IOC = 0;
> diff --git a/util.h b/util.h
> new file mode 100644
> index 0000000..4be9bc8
> --- /dev/null
> +++ b/util.h
> @@ -0,0 +1,67 @@
> +/*
> + * Linux RAID Management Application
> + * Copyright (c) 2009 Intel Corporation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> it
> + * under the terms of the GNU General Public License version 2 as
> published
> + * by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warrany of
> MERCHANTABILITY
> + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
> License
> + * for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> along
> + * with this program; if not, write to the Free Software Foundation,
> Inc.,
> + * 51 Franklin Street, Fifhth Floor, Boston, MA 02110-1301, USA.
> + */
> +
> +#ifndef _UTIL_H_INCLUDED
> +#define _UTIL_H_INCLUDED
> +
> +/* Define PATH_MAX in case we don't use GLIBC or the standard library
> does
> + not have PATH_MAX defined. Assume maximum path length is 4K
> characters. */
> +#ifndef PATH_MAX
> +#define PATH_MAX 4096
> +#endif /* PATH_MAX */
> +
> +/**
> + * @brief Wrapper macro for __str_fmt function.
> + *
> + * This macro makes use of __str_fmt() function easier. Use this macro
> with
> + * caution and only for arrays. Do not use this macro with pointers
> because
> + * the result might be unexpected.
> + *
> + * @param[in] __buf Destination buffer.
> + * @param[in] __fmt Format string.
> + *
> + * @return The return value of this marco is the same as for
> __str_fmt()
> + * function. See description of __str_fmt() for more details.
> + */
> +#define str_fmt(__buf, __fmt, ...) \
> + __str_fmt((char *)(__buf), sizeof(__buf), (const char *)(__fmt), ##
> __VA_ARGS__)
> +
> +/**
> + * @brief Formats a string according to pattern.
> + *
> + * This is printf() like function which formats a text buffer according
> to
> + * format pattern. The function stores the result of formating in a
> destination
> + * buffer. The destrination buffer is always null terminated even if
> result
> + * does not fit in it. See description of printf() function for details
> on how
> + * to format the output.
> + *
> + * @param[in] buf Pointer to destination buffer where
> the
> + * result of formating will be stored.
> + * @param[in] buf_size The capacity of destination buffer
> including
> + * null ('\0') character.
> + * @param[in] fmt Pointer to buffer containging the
> pattern.
> + *
> + * @return If successful the function returns number of characters put
> in
> + * destination buffer, otherwise the function returns -1. Check
> errno
> + * variable to get details about the cause of an error.
> + */
> +extern int __str_fmt(char *buf, size_t buf_size, const char *fmt, ...)
> + __attribute__((format(printf, 3, 4)));
I don't like that this function silently does not work with pointers,
and its name belies the fact that it does checking in addition to
formatting.
Perhaps something can be done with gcc builtins for this case. Have
you looked into __builtin_object_size, __builtin___snprintf_chk and
friends. The goal being to use these builtins to:
1/ Get a compile time warning when an overflow is detected (currently
supported by the builtins)
2/ Get a compile time warning if the bounds cannot be checked at
compile time (would need some investigation)
--
Dan
[1] http://userweb.kernel.org/~akpm/stuff/tpp.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
