Messages in this thread | | | Date | Sat, 19 Jul 2008 15:41:15 +0800 | From | JiSheng Zhang <> | Subject | Re: PATCH] firewire: add padding to some struct |
| |
On Fri, 18 Jul 2008 17:27:44 +0200 Mikael Pettersson <mikpe@it.uu.se> wrote:
> JiSheng Zhang writes: > > Hi, > > >From: Stefan Richter <stefanr@s5r6.in-berlin.de> > > >Reply-To: > > >To: JiSheng Zhang <jszhang3@mail.ustc.edu.cn> > > >Subject: Re: PATCH] firewire: add padding to some struct > > >Date:Fri, 18 Jul 2008 13:38:25 +0200 > > > > > >JiSheng Zhang wrote: > > > > If p is a pointer to struct fw_cdev_event_response), p->data will point to > > the > > > > padding data rather than the right place, it will cause problem under some > > Define "the right place". If p->data[] isn't the place for the data, > then something's seriously wrong with either the producer or the > consumer of that data -- or the data type definition if either is HW. > > > > > platforms. For example, in the function handle_device_event of > > libraw1394(ported > > > > to juju stack): > > > > ..... > > > > case FW_CDEV_EVENT_RESPONSE: > > > > rc = u64_to_ptr(u->response.closure); > > > > if (rc->data != NULL) > > > > memcpy(rc->data, u->response.data, rc->length);//here it will lost the last > > four > > > > bytes > > > > errcode = juju_to_raw1394_errcode(u->response.rcode); > > > > ..... > > > > > > > > Although this problem can be solved by add the offset to the pointer, but the > > > > member:__u32 data[0] lost its original meaning. > > > > > > I don't understand what the problem is. As long as both kernel and > > > library use "response.data" or "&response + offsetof(typeof(response), > > > data)", they will write and read at the correct location. > > > > > This patch can fix the problem while not changing the struct definition. > > > > > > Thanks in advance, > > JiSheng > > > > --- old/drivers/firewire/fw-cdev.c 2008-07-14 05:51:29.000000000 +0800 > > +++ new/drivers/firewire/fw-cdev.c 2008-07-18 20:20:45.841328585 +0800 > > @@ -382,9 +382,9 @@ > > > > response->response.type = FW_CDEV_EVENT_RESPONSE; > > response->response.rcode = rcode; > > - queue_event(client, &response->event, > > - &response->response, sizeof(response->response), > > - response->response.data, response->response.length); > > + queue_event(client, &response->event, &response->response, > > + sizeof(response->response) + response->response.length, > > + NULL, 0); > > } > > Neither of these look correct. > If sizeof(struct ...) != offsetof(struct ..., data) as you claim is possible, > then the old code will copy too much to/from ->response but the correct amount > to/from ->response.data, and the new code will copy too much to/from ->response.data. The old code will copy 4 extra bytes totally on some platforms, the new code is correct. The old one queue like this: struct ...(excluding the padding bytes)|4 padding bytes|4 padding bytes|data > > That's why C has offsetof(): > > queue_event(client, &response->event, > &response->response, > offsetof(typeof(*response->responce), data), // I don't know the struct name > response->response.data, response->response.length);
Bye, JiSheng
| |