From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa0-f50.google.com (mail-oa0-f50.google.com [209.85.219.50]) by befuddled.reisers.ca (Postfix) with ESMTPS id F223A1EF6D5 for ; Thu, 9 May 2013 19:50:25 -0400 (EDT) Received: by mail-oa0-f50.google.com with SMTP id l20so1247905oag.37 for ; Thu, 09 May 2013 16:50:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:date:from:to:subject:message-id:mail-followup-to :mime-version:content-type:content-disposition:user-agent; bh=LTS5aQbzDeL3e++idhycvENshYLeNrkgAjlaJ8iHFOo=; b=dOJyEzy7FN87iD94XH2wOJjsRh2iR8aFeePrgIXr7bJEnoo+uetcLNctG0V5Wqwzd5 Q708KIsPVrJYhpu/NifHbcnSe9WPiRVakJykEYJZWHjG+vJrg5Ru3PJmC4RoioLWdvVL f8mh9hfhu0WI2Ti6onehKrsBF3uHd06iqJ6RgTQklyjzSYzZhwMjpeJ20cfrsOC1p/wr yVUlNr2qVxR0U/6zOcUpA3N5l66gH2CHQN683m//xQqlZbWIYdNsNnBE/AE9a+M9l0Je BA2X0Ic/V+ch/EkVVwUwPLMiKSdLLg/SBSOWhGuwQNwrBSSZiXA1GR2TsfI0VDhkjdT5 F5Pg== X-Received: by 10.60.144.38 with SMTP id sj6mr5713518oeb.111.1368143425211; Thu, 09 May 2013 16:50:25 -0700 (PDT) Received: from linux1 (cpe-76-187-91-128.tx.res.rr.com. [76.187.91.128]) by mx.google.com with ESMTPSA id dt3sm301661obb.12.2013.05.09.16.50.22 for (version=TLSv1.2 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 09 May 2013 16:50:24 -0700 (PDT) Received: by linux1 (sSMTP sendmail emulation); Thu, 09 May 2013 18:50:12 -0500 Date: Thu, 9 May 2013 18:50:12 -0500 From: William Hubbs To: speakup@linux-speakup.org Subject: Fwd: Re: speakup issues Message-ID: <20130509235012.GC2123@linux1> Mail-Followup-To: speakup@linux-speakup.org MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: speakup@linux-speakup.org X-Mailman-Version: 2.1.15 Precedence: list Reply-To: "Speakup is a screen review system for Linux." List-Id: "Speakup is a screen review system for Linux." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 May 2013 23:50:26 -0000 ----- Forwarded message from Dan Carpenter ----- > Date: Fri, 10 May 2013 01:49:12 +0300 > From: Dan Carpenter > To: William Hubbs > Cc: devel@driverdev.osuosl.org > Subject: Re: speakup issues > > Btw, this code isn't horrible. It uses small functions and it's > readable. It's just needs small cleanups throughout... > > Get rid of the global variable called "synth". That's a bad name > for a global and it's shadowed by local variables named "synth" so > it creates confusion. > > Don't do "pr_warn("synth probe\n");" on the success path. > > As much as possible get rid of forward declarations. > > Some of the 80 character line breaks were done in a funny way. > > What is this code? > #if defined(__powerpc__) || defined(__alpha__) > cval >>= 8; > #else /* !__powerpc__ && !__alpha__ */ > cval >>= 4; > #endif /* !__powerpc__ && !__alpha__ */ > > Delete commented out code. > > The file scoped variable "timeouts" is only used in one function. > It could be declared as a static variable locally in that function. > > /* > * this is cut&paste from 8250.h. Get rid of the structure, the definitions > * and this whole broken driver. > */ > > We go to a lot of effort to print out this message which just tells > you that adds no information: > > if (failed) { > pr_info("%s: not found\n", synth->long_name); > return -ENODEV; > } > > synth_add() has a memory corrupting off-by-one bug. > > The code returns -1 (which means "permission denied") instead of > returning standard error codes. > > The author of this code doesn't like break statements and uses > compound conditions to avoid them. > > 426 for (i = 0; i < MAXSYNTHS && synths[i] != NULL; i++) > 427 /* synth_remove() is responsible for rotating the array down */ > 428 if (in_synth == synths[i]) { > 429 mutex_unlock(&spk_mutex); > 430 return 0; > 431 } > 432 if (i == MAXSYNTHS) { > 433 pr_warn("Error: attempting to add a synth past end of array\n"); > 434 mutex_unlock(&spk_mutex); > 435 return -1; > 436 } > > Unless there is a special reason then for loops should be written > in the canonical format. Use curly braces for readability when > there is a multi-line loop even if they are not needed > syntactically. > > for (i = 0; i < ARRAY_SIZE(synths); i++) { > /* synth_remove() is responsible for rotating the array down */ > if (synths[i] == in_synth) { > mutex_unlock(&spk_mutex); > return 0; > } > if (synths[i] == NULL) > break; > } > if (i == ARRAY_SIZE(synths)) { > pr_warn("Error: attempting to add a synth past end of array\n"); > mutex_unlock(&spk_mutex); > return -ENOMEM; > } > > Pretty much where ever you look there is something to clean up. > Fortunately, it's mostly small things. > > Sparse has a few things it complains about. > > regards, > dan carpenter ----- End forwarded message -----