2006-05-02 08:06:22

by Michael Holzheu

[permalink] [raw]
Subject: Re: [PATCH] s390: Hypervisor File System

Hi Joern,

J?rn Engel <[email protected]> wrote on 04/28/2006 07:43:02 PM:
> On Fri, 28 April 2006 19:36:30 +0200, Michael Holzheu wrote:
> > Andrew Morton <[email protected]> wrote on 04/28/2006 11:56:21 AM:
> > > Michael Holzheu <[email protected]> wrote:
> >
> > > > +static int diag224_idx2name(int index, char *name)
> > > > +{
> > > > + memcpy(name, diag224_cpu_names + ((index + 1) * 16), 16);
> > > > + name[16] = 0;
> > >
> > > Should this be "15"? I guess not...
> >
> > No bug, our strings here have 16 characters and are not
> > 0 terminated.
>
> Hmm. TMP_SIZE is defined to 64 and used for buffers allocated on the
> stack. It is not too excessive, but in this case 17 would definitely
> be enough. Not sure if it's worth going through.
>
> J?rn

Since I use the buffers with size TMP_SIZE also for other purposes
in the same functions, where the cpu types are accessed,
I think, it is not useful to define a second buffer with size 17.

But I think it is better to have a define for the buffer size
of cpu names. something like:

#define LPAR_NAME_LEN 8 /* lpar name len in diag 204 data
*/
+#define CPU_NAME_LEN 16 /* type name len of a cpu in
diag224 name table */
#define TMP_SIZE 64 /* size of temporary buffers */

static int diag224_idx2name(int index, char *name)
{
- memcpy(name, diag224_cpu_names + ((index + 1) * 16), 16);
- name[16] = 0;
+ memcpy(name, diag224_cpu_names + ((index + 1) * CPU_NAME_LEN),
+ CPU_NAME_LEN);
+ name[CPU_NAME_LEN] = 0;
strstrip(name);
return 0;
}

Michael