On 17:14, Neil Brown wrote:
> On Tue, 8 Dec 2009 11:29:43 +0100
> Andre Noll <maan@xxxxxxxxxxxxxxx> wrote:
>
> Thanks for the very thorough review.
>
> I think I have address all your issues - see below.
Jup, looks fine to me now.
>
> > > + /* No bitmap, OK to set a location */
> > > + long long offset;
> > > + if (strncmp(buf, "none", 4) == 0)
> > > + /* nothing to be done */;
> > > + else if (strncmp(buf, "file:", 5) == 0) {
> > > + /* Not supported yet */
> > > + return -EINVAL;
> > > + } else {
> > > + int rv;
> > > + if (buf[0] == '+')
> > > + rv = strict_strtoll(buf+1, 10, &offset);
> > > + else
> > > + rv = strict_strtoll(buf, 10, &offset);
> > > + if (rv)
> > > + return rv;
> > > + mddev->bitmap_info.offset = offset;
> >
> > Introducing a sanity check here would probably save some sysadmin's
> > feet.
>
> What sort of sanity check did you have in mind. When we come to
> write out bitmap data a sanity check is performed and if there is
> any overlap the bitmap is disabled.
I thought of a simple check for overlaps, not being aware of the fact
that such a check is already done in write_sb_page() of bitmap.c. Doing
the check there is obviously good as well. The only difference is
that for invalid offset values the bitmap is accepted (i.e. setting
the bitmap with mdadm succeeds) but then kicked immediately.
> > Also, if the timeout is being set while the array is stopped, the
> > command succeeds but has no effect (as mddev->thread is NULL). Maybe
> > it's better to return an error in this case.
>
> No - bitmap_info.daemon_sleep is always set. This is the important part of
> the function. This "if (mddev->thread) {" branch is just to ensure it takes
> effect instantly instead of soon.
Ah, I see. Thanks for the explanation.
Andre
--
The only person who always got his work done by Friday was Robinson Crusoe
Attachment:
signature.asc
Description: Digital signature
