Hi Dan,
Thank you for your comments. All you suggestions I will incorporate in
next version of a patch. Please see my comments below.
> 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].
Definitely I should follow your suggestion.
> > @@ -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.
Yes, that is my intention and if you are ok with this I would keep it.
> > @@ -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.
These are Intel guidelines, but I clarify this with Intel SQA.
> > + * 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.
You right. I assumed no one will request buffers bigger then INT_MAX,
but this is wrong assumption. I agree the better solution is to let
vnsprintf() function to resolve this issue.
> > +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)
I will investigate this and correct the patch.
--
Artur
--
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
