Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 21 Nov 2011 20:29:57 +1100 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        mdf@freebsd.org
Cc:        src-committers@freebsd.org, Pawel Jakub Dawidek <pjd@freebsd.org>, Garrett Cooper <yanegomi@gmail.com>, Alexander Motin <mav@freebsd.org>, svn-src-head@freebsd.org, svn-src-all@freebsd.org
Subject:   Re: svn commit: r227473 - head/sbin/geom/class/multipath
Message-ID:  <20111121200813.E1158@besplex.bde.org>
In-Reply-To: <CAMBSHm_HisiWu8OtueNdrMbZEV%2B%2BRR0xO0UCeQDmy0W_swevAg@mail.gmail.com>
References:  <201111122001.pACK1UML059238@svn.freebsd.org> <CAGH67wScn5fvZ-ZFkcPDTkhxF9K2isWMQHA7wsHiy5PihidZCA@mail.gmail.com> <20111113220817.GC1677@garage.freebsd.pl> <CAGH67wSwG_Kc-dr-jbqKH2Z0K6yUN4625RfH-Y=8_KGdvTDyzQ@mail.gmail.com> <CAMBSHm_HisiWu8OtueNdrMbZEV%2B%2BRR0xO0UCeQDmy0W_swevAg@mail.gmail.com>

next in thread | previous in thread | raw e-mail | index | archive | help
  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--0-825119736-1321867797=:1158
Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed
Content-Transfer-Encoding: QUOTED-PRINTABLE

On Sun, 13 Nov 2011 mdf@freebsd.org wrote:

> On Sun, Nov 13, 2011 at 2:46 PM, Garrett Cooper <yanegomi@gmail.com> wrot=
e:
>> On Sun, Nov 13, 2011 at 2:08 PM, Pawel Jakub Dawidek <pjd@freebsd.org> w=
rote:
>>> On Sat, Nov 12, 2011 at 12:16:23PM -0800, Garrett Cooper wrote:
>>>> On Sat, Nov 12, 2011 at 12:01 PM, Alexander Motin <mav@freebsd.org> wr=
ote:
>>>>> Author: mav
>>>>> Date: Sat Nov 12 20:01:30 2011
>>>>> New Revision: 227473
>>>>> URL: http://svn.freebsd.org/changeset/base/227473
>>>>>
>>>>> Log:
>>>>> =A0Fix build on some archs after r227464.
>>>>>
>>>>> Modified:
>>>>> =A0head/sbin/geom/class/multipath/geom_multipath.c
>>>>>
>>>>> Modified: head/sbin/geom/class/multipath/geom_multipath.c
>>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D
>>>>> --- head/sbin/geom/class/multipath/geom_multipath.c =A0 =A0 Sat Nov 1=
2 19:55:48 2011 =A0 =A0 =A0 =A0(r227472)
>>>>> +++ head/sbin/geom/class/multipath/geom_multipath.c =A0 =A0 Sat Nov 1=
2 20:01:30 2011 =A0 =A0 =A0 =A0(r227473)
>>>>> @@ -133,7 +133,8 @@ mp_label(struct gctl_req *req)
>>>>> =A0 =A0 =A0 =A0uint8_t *sector, *rsector;
>>>>> =A0 =A0 =A0 =A0char *ptr;
>>>>> =A0 =A0 =A0 =A0uuid_t uuid;
>>>>> - =A0 =A0 =A0 uint32_t secsize =3D 0, ssize, status;
>>>>> + =A0 =A0 =A0 ssize_t secsize =3D 0, ssize;
>>>>> + =A0 =A0 =A0 uint32_t status;
>>>>> =A0 =A0 =A0 =A0const char *name, *name2, *mpname;
>>>>> =A0 =A0 =A0 =A0int error, i, nargs, fd;
>>>>>
>>>>> @@ -161,8 +162,8 @@ mp_label(struct gctl_req *req)
>>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0disksize =3D msize;
>>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else {
>>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (secsize !=3D ssize=
) {
>>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 gctl_er=
ror(req, "%s sector size %u different.",
>>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 name, ssize);
>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 gctl_er=
ror(req, "%s sector size %ju different.",
>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 name, (intmax_t)ssize);
>>>>
>>>> Shouldn't that be uintmax_t, not intmax_t ?
>>>
>>> No, ssize_t is signed. Although the best would be to use %zd for
>>> ssize_t.
>>
>> Thanks... Missed the leading s.
>
> ... except that the cast and conversion specifier aren't in sync.  The
> cast is signed; the conversion specifier is unsigned.  %zd is still
> best, since it's the conversion specifier for the variable's type.
> Next best is %jd and cast to intmax_t, since the signedness is
> preserved.

But ssize_t is still a nonsense type for representing a sector size.
     ssize_t is for the return type of read(2) and write(2) and not much
     more.  On 64-bit arches it is (bogusly) int64_t, although read() and
     write() are still limited to INT_MAX for historical reasons (buggy
     standards may require ssize_t to have this wrong type and SSIZE_MAX
     to have the corresponding wrong value INT64_MAX (equal to the maximum
     value representable by this wrong type and not equal to the maximum
     value that read() and write() actually support).  These bugs were
     missing in V7 where read() and write() took and returned plain int
     sizes.  Portable code must still call them with a size <=3D INT_MAX,
     else it may break, for example on FreeBSD.
When ssize_t is misused for something unrelated to read() and write(),
it mainly gives printf format complications since it tends to be larger
than necessary.  Here it gives a printf format complication of a cast
to intmax_t, and a printf format error since intmax_t doesn't match
the format %ju, and it gives the nonsense that sectors of size >=3D 2GB
are "supported" on 64-bit systems but not on 32-bit systems.  Testing
this support is difficult even with virtual disks.

> But in the end it doesn't matter a lot since whatever is printed in
> the error message, it's likely derivable from reading the code what
> the actual value used was.

Even when the value is truncated, provided the behaviour is defined or
benign.

Bruce
--0-825119736-1321867797=:1158--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111121200813.E1158>