Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1610883

Re: [PATCH] Revert "md: raid1: use bio helper in process_checks()"

Show all headers | View raw

On Tue, Mar 28, 2017 at 1:42 PM, Ming Lei <tom.leiming@gmail.com> wrote:
> On Tue, Mar 28, 2017 at 7:35 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Tue, Mar 28, 2017 at 12:44 PM, Ming Lei <tom.leiming@gmail.com> wrote:
>>> On Tue, Mar 28, 2017 at 5:49 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> Commit 60928a91b0b3 ("md: raid1: use bio helper in process_checks()")
>>>> is probably correct, but I get a new compile-time warning after
>>>> it, and have trouble understanding what it fixes:
>>>>
>>>> drivers/md/raid1.c: In function 'sync_request_write':
>>>> drivers/md/raid1.c:2172:9: error: 'page_len$' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>>>      if (memcmp(page_address(ppages[j]),
>>>>          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>          page_address(spages[j]),
>>>>          ~~~~~~~~~~~~~~~~~~~~~~~~
>>>>          page_len[j]))
>>>>          ~~~~~~~~~~~~
>>>> drivers/md/raid1.c:2160:7: note: 'page_len$' was declared here
>>>>    int page_len[RESYNC_PAGES];
>>>>        ^~~~~~~~
>>>>
>>>> This reverts it to resolve the warning.
>>>
>>> Please try the following patch:
>>>
>>>  https://lkml.org/lkml/2017/3/28/126
>>
>>
>> That patch will certainly shut up the warning, but will also prevent
>> the compiler from warning when the function gets changed in some
>> way that actually leads to an uninitialized use of the page_len array,
>
> Why do you think that it leads to an uninitialized use of the page_len array?

What I meant is that a future change to the function might cause
another bug to go unnoticed later.

> The following code does initialize the array well enough for future use:
>
>                bio_for_each_segment_all(bi, sbio, j)
>                        page_len[j] = bi->bv_len;
>
> That is why we don't need to initialize the array explicitly, but just
> for killing the warning.

It's also a little less clear why that is safe than the original code:
We rely on sbio->bi_vcnt to be the same as vcnt, but it requires
careful reading of the function to see that this is always true.
gcc warns because it cannot prove this to be the case, so if
something changed here, it's likely that this would also not
get noticed.

     Arnd

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH] Revert "md: raid1: use bio helper in process_checks()" Arnd Bergmann <arnd@arndb.de> - 2017-03-28 12:00 +0200
  Re: [PATCH] Revert "md: raid1: use bio helper in process_checks()" Ming Lei <tom.leiming@gmail.com> - 2017-03-28 12:50 +0200
    Re: [PATCH] Revert "md: raid1: use bio helper in process_checks()" Arnd Bergmann <arnd@arndb.de> - 2017-03-28 13:40 +0200
      Re: [PATCH] Revert "md: raid1: use bio helper in process_checks()" Ming Lei <tom.leiming@gmail.com> - 2017-03-28 13:50 +0200
        Re: [PATCH] Revert "md: raid1: use bio helper in process_checks()" Arnd Bergmann <arnd@arndb.de> - 2017-03-28 15:30 +0200
          Re: [PATCH] Revert "md: raid1: use bio helper in process_checks()" Ming Lei <tom.leiming@gmail.com> - 2017-03-28 17:10 +0200
            Re: [PATCH] Revert "md: raid1: use bio helper in process_checks()" Arnd Bergmann <arnd@arndb.de> - 2017-03-28 17:50 +0200
              Re: [PATCH] Revert "md: raid1: use bio helper in process_checks()" Ming Lei <tom.leiming@gmail.com> - 2017-03-28 18:20 +0200
            Re: [PATCH] Revert "md: raid1: use bio helper in process_checks()" Wols Lists <antlists@youngman.org.uk> - 2017-03-28 18:00 +0200
              Re: [PATCH] Revert "md: raid1: use bio helper in process_checks()" Henrique de Moraes Holschuh <hmh@hmh.eng.br> - 2017-04-02 00:00 +0200

csiph-web