headers
Rob Landley | 1 May 2006 21:19

Re: [PATCH] hdparm size reduction

On Saturday 29 April 2006 3:14 pm, Tito wrote:
> Hi,
> this patch applies on top of the last applied patches of Denis
> and removes some dead code in identify(),
> some obsolete  code in process_dev() and some obsolete functions
> (no_scsi(), no_xt(), no_scsi_no_xt()) as in latest hdparm6.6.
>
> Please apply.

The first two hunks were rejected.

What's the deal with chksum?  In the previous code, it was never being set (it 
was only set in the dead code that was never reached), and now it's tested in 
one place where they only care that' it's zero.  There's another place that 
we print it and say it's correct, but we never actually _test_ it so how do 
we know...?

> Ciao,
> Tito

Rob
--

-- 
Never bet against the cheap plastic solution.
Permalink | Reply |
headers
Tito | 2 May 2006 08:07
Picon
Favicon

Re: [PATCH] hdparm size reduction

On Monday 1 May 2006 21:19, you wrote:
> On Saturday 29 April 2006 3:14 pm, Tito wrote:
> > Hi,
> > this patch applies on top of the last applied patches of Denis
> > and removes some dead code in identify(),
> > some obsolete  code in process_dev() and some obsolete functions
> > (no_scsi(), no_xt(), no_scsi_no_xt()) as in latest hdparm6.6.
> >
> > Please apply.
> 
> The first two hunks were rejected.
> 
> What's the deal with chksum?  In the previous code, it was never being set (it 
> was only set in the dead code that was never reached), and now it's tested in 
> one place where they only care that' it's zero.  There's another place that 
> we print it and say it's correct, but we never actually _test_ it so how do 
> we know...?
Maybe here (in the rejected hunks)? 

	/* calculate checksum over all bytes */
	for(ii = GEN_CONFIG; ii<=INTEGRITY; ii++) {
		chksum += val[ii] + (val[ii] >> 8);
	}

BTW: identify() could be a candidate for an ATTRIBUTE_NORETURN?

Ciao,
Tito
> > Ciao,
> > Tito
(Continue reading)

Permalink | Reply |
headers
Rob Landley | 2 May 2006 15:43

Re: [PATCH] hdparm size reduction

On Tuesday 02 May 2006 2:07 am, Tito wrote:
> On Monday 1 May 2006 21:19, you wrote:
> > On Saturday 29 April 2006 3:14 pm, Tito wrote:
> > > Hi,
> > > this patch applies on top of the last applied patches of Denis
> > > and removes some dead code in identify(),
> > > some obsolete  code in process_dev() and some obsolete functions
> > > (no_scsi(), no_xt(), no_scsi_no_xt()) as in latest hdparm6.6.
> > >
> > > Please apply.
> >
> > The first two hunks were rejected.
> >
> > What's the deal with chksum?  In the previous code, it was never being
> > set (it was only set in the dead code that was never reached), and now
> > it's tested in one place where they only care that' it's zero.  There's
> > another place that we print it and say it's correct, but we never
> > actually _test_ it so how do we know...?
>
> Maybe here (in the rejected hunks)?
>
> 	/* calculate checksum over all bytes */
> 	for(ii = GEN_CONFIG; ii<=INTEGRITY; ii++) {
> 		chksum += val[ii] + (val[ii] >> 8);
> 	}

I checked in the non-rejected bits, with one manual fixup.  It didn't seem to 
make anything worse. :)

> BTW: identify() could be a candidate for an ATTRIBUTE_NORETURN?
(Continue reading)

Permalink | Reply |
headers
Tito | 2 May 2006 21:29
Picon
Favicon

Re: [PATCH] hdparm size reduction

On Tuesday 2 May 2006 15:43, Rob Landley wrote:
> On Tuesday 02 May 2006 2:07 am, Tito wrote:
> > On Monday 1 May 2006 21:19, you wrote:
> > > On Saturday 29 April 2006 3:14 pm, Tito wrote:

snip 

> 
> > BTW: identify() could be a candidate for an ATTRIBUTE_NORETURN?
> 
> Could be.  Want to send a new patch with a respin of the rejected hunks plus 
> this?

Here it is: hdparm07.patch!!!

Ciao,
Tito
> 
> Rob
Attachment (hdparm07.patch): text/x-diff, 1171 bytes
_______________________________________________
busybox mailing list
busybox <at> busybox.net
http://busybox.net/cgi-bin/mailman/listinfo/busybox
Permalink | Reply |
headers
Rob Landley | 3 May 2006 00:50

Re: [PATCH] hdparm size reduction

On Tuesday 02 May 2006 3:29 pm, Tito wrote:
> On Tuesday 2 May 2006 15:43, Rob Landley wrote:
> > On Tuesday 02 May 2006 2:07 am, Tito wrote:
> > > On Monday 1 May 2006 21:19, you wrote:
> > > > On Saturday 29 April 2006 3:14 pm, Tito wrote:
>
> snip
>
> > > BTW: identify() could be a candidate for an ATTRIBUTE_NORETURN?
> >
> > Could be.  Want to send a new patch with a respin of the rejected hunks
> > plus this?
>
> Here it is: hdparm07.patch!!!

My if (BB_BIG_ENDIAN) hunk was there to avoid the memcpy when we didn't need 
to do it.  (Hopefully the compiler is smart enough to realize that val can 
optimized away when it's just an alias for another variable.)

You removed that.  Was there a reason?

Rob 
--

-- 
Never bet against the cheap plastic solution.
Permalink | Reply |
headers
Tito | 3 May 2006 08:46
Picon
Favicon

Re: [PATCH] hdparm size reduction

On Wednesday 3 May 2006 00:50, Rob Landley wrote:
> On Tuesday 02 May 2006 3:29 pm, Tito wrote:
> > On Tuesday 2 May 2006 15:43, Rob Landley wrote:
> > > On Tuesday 02 May 2006 2:07 am, Tito wrote:
> > > > On Monday 1 May 2006 21:19, you wrote:
> > > > > On Saturday 29 April 2006 3:14 pm, Tito wrote:
> >
> > snip
> >
> > > > BTW: identify() could be a candidate for an ATTRIBUTE_NORETURN?
> > >
> > > Could be.  Want to send a new patch with a respin of the rejected hunks
> > > plus this?
> >
> > Here it is: hdparm07.patch!!!
> 
> My if (BB_BIG_ENDIAN) hunk was there to avoid the memcpy when we didn't need 
> to do it.  (Hopefully the compiler is smart enough to realize that val can 
> optimized away when it's just an alias for another variable.)
> 
> You removed that.  Was there a reason?

Just copy and paste from hdparm 6.6+

Tito

 > Rob
Permalink | Reply |
headers
Rob Landley | 3 May 2006 20:13

Re: [PATCH] hdparm size reduction

On Wednesday 03 May 2006 2:46 am, Tito wrote:

> > > Here it is: hdparm07.patch!!!
> >
> > My if (BB_BIG_ENDIAN) hunk was there to avoid the memcpy when we didn't
> > need to do it.  (Hopefully the compiler is smart enough to realize that
> > val can optimized away when it's just an alias for another variable.)
> >
> > You removed that.  Was there a reason?
>
> Just copy and paste from hdparm 6.6+

The other thing is that the big endian test was doing a swab before, and now 
it isn't.  You still have a buffer, but you always memcpy straight to the 
buffer unmodified.  So either your hdparm07 breaks big endian systems, or the 
swab was unnecessarily (strongly inplying that the _old_ hdparm wouldn't have 
worked on big endian systems).

I'm going to guess that the old one worked and the swab is still necessary.  
Care to express an opinion?

I should set up an ubuntu PPC system under qemu so I can test this...

Rob
--

-- 
Never bet against the cheap plastic solution.
Permalink | Reply |
headers
Tito | 3 May 2006 22:06
Picon
Favicon

Re: [PATCH] hdparm size reduction

On Wednesday 3 May 2006 20:13, Rob Landley wrote:
> On Wednesday 03 May 2006 2:46 am, Tito wrote:
> 
> > > > Here it is: hdparm07.patch!!!
> > >
> > > My if (BB_BIG_ENDIAN) hunk was there to avoid the memcpy when we didn't
> > > need to do it.  (Hopefully the compiler is smart enough to realize that
> > > val can optimized away when it's just an alias for another variable.)
> > >
> > > You removed that.  Was there a reason?
> >
> > Just copy and paste from hdparm 6.6+
> 
> The other thing is that the big endian test was doing a swab before, and now 
> it isn't.  You still have a buffer, but you always memcpy straight to the 
> buffer unmodified.  So either your hdparm07 breaks big endian systems, or the 
> swab was unnecessarily (strongly inplying that the _old_ hdparm wouldn't have 
> worked on big endian systems).
> 
> I'm going to guess that the old one worked and the swab is still necessary.  
> Care to express an opinion?

No opinion and no way to test it.

Tito
> 
> I should set up an ubuntu PPC system under qemu so I can test this...
> 
> Rob
(Continue reading)

Permalink | Reply |
headers
Rob Landley | 4 May 2006 00:22

Re: [PATCH] hdparm size reduction

On Wednesday 03 May 2006 4:06 pm, Tito wrote:
> > I'm going to guess that the old one worked and the swab is still
> > necessary. Care to express an opinion?
>
> No opinion and no way to test it.
>
> Tito
>
> > I should set up an ubuntu PPC system under qemu so I can test this...
> >
> > Rob

Oddly, the ubuntu live cd for PPC will boot under qemu, but the install cd 
won't.  Not sure why...

Rob
--

-- 
Never bet against the cheap plastic solution.
Permalink | Reply |
headers
Tito | 4 May 2006 08:14
Picon
Favicon

Re: [PATCH] hdparm size reduction

On Thursday 4 May 2006 00:22, Rob Landley wrote:
> On Wednesday 03 May 2006 4:06 pm, Tito wrote:
> > > I'm going to guess that the old one worked and the swab is still
> > > necessary. Care to express an opinion?
> >
> > No opinion and no way to test it.
> >
> > Tito
> >
> > > I should set up an ubuntu PPC system under qemu so I can test this...
> > >
> > > Rob
> 
> Oddly, the ubuntu live cd for PPC will boot under qemu, but the install cd 
> won't.  Not sure why...

Isn't there a way to install ubuntu from the livecd or test hdparm while running the live cd?

Tito

> Rob
Permalink | Reply |

Gmane