Message-ID: <20150930073338.GA13284@kroah.com>
Date:	Wed, 30 Sep 2015 09:33:38 +0200
From:	Greg KH <gregkh@...uxfoundation.org>
To:	Alexander Kapshuk <alexander.kapshuk@...il.com>
Cc:	linux-kernel@...r.kernel.org, Randy Dunlap <rdunlap@...radead.org>
Subject: Re: [PATCH] ver_linux: uniform output across various linux distros

On Tue, Sep 29, 2015 at 09:08:20PM +0300, Alexander Kapshuk wrote:
> On Tue, Sep 29, 2015 at 7:43 PM, Greg KH <gregkh@...uxfoundation.org> wrote:
> > On Tue, Sep 29, 2015 at 06:36:12PM +0300, Alexander Kapshuk wrote:
> >> I've reread 'Documentation/SubmittingPatches', and opted for method 1
> >> to create the patch.
> >>
> >> When you mentioned about the patch changing the formatting, were you
> >> referring to the formatting of the actual script, or the output the
> >> script generated? If it's the former, I'm not sure I'm clear on how to
> >> keep separate the changes made to the code base from the changes in
> >> the formatting of the new code base introduced by these changes. If
> >> it's the latter, the overall output generated by the script is mostly
> >> identical to that generated by the original implementation.
> >>
> >> If the patch below is still off the mark, I apologise for that, and
> >> would appreciate being pointed out what else needs changing, when you
> >> have the time. I understand you having more pressing matters to attend
> >> to.
> >>
> >> Signed-off-by: Alexander Kapshuk <alexander.kapshuk@...il.com>
> >
> > This is the "oddest" changelog comments I've ever seen for a patch, it
> > doesn't describe what it is doing at all :)
> >
> > Please fix up, and resend the whole thing correctly, so we can review
> > that.
> >
> > thanks,
> >
> > greg k-h
> 
> 
> O, boy. Will I ever get this right :-(
> Sorry for being such a nuisance.
> Here we go...

Heh, in the future, all of this above here needs to be removed, it
wouldn't look good in a changelog, right?

But anyway, comments below on your patch:

> Having run 'scripts/ver_linux' on my Gentoo system, as well as having
> looked through some recent bug reports on the kernel bugzilla website
> showing the output of the script in question, I have observed that
> the output is not accurate across various distros. While the current
> implementation of the script expects version numbers to be found
> in particular fields, some of the utilities invoked by the script,
> output their version information in varying formats, which results in
> the script displaying information other than the version number.
> 
> The proposed implementation relies mostly on sed to detect the version
> numbers more accurately in a more general way.
> 
> Running the patched version of the script on the distros below resulted
> in accurate and uniform output.
> 
> Gentoo Linux
> Debian 6.0.10
> Oracle Linux Server release 7.1
> Arch Linux
> openSuSE 13.2
> 
> The items left unchanged are those I did not have access to. I would
> be willing to work on those too, if supplied the output of the affected
> commands whose format differs based on the distro.
> 
> -------------------------------------
> A few comments are in order.
> 
> /usr/src/linux/scripts/ver_linux.orig:22,26
> The current implementation outputs the version of 'util-linux' by
> calling 'fdformat', which had not been found installed on most of the
> test systems available to me. As 'mount' is a part of the 'util-linux'
> package, the proposed implementation invokes 'mount' only, and unifies
> the lines above into a single block.
> 
> /usr/src/linux/scripts/ver_linux.orig:77,83
> 'loadkeys -h' does not output the versionning number across all the
> test systems available to me, while 'loadkeys -V' does. The proposed
> implementation calls 'loadkeys -V' only once, and outputs the versioning
> info acquired for both 'Kdb' and 'console-tools'.
> 
> /usr/src/linux/scripts/ver_linux.orig:90
> None of the test systems I have access to seem to have 'udevinfo'
> installed. Perhaps, 'udevinfo' is the former name for something that
> could now be substituted by 'udevadm --version'.
> -------------------------------------
> Sample output of the current implementation on Gentoo Linux:
> 
> If some fields are empty or look unusual you may have an old version.
> Compare to the current minimal requirements in Documentation/Changes.
> 
> Linux box0 4.2.1-vanilla #1 SMP Thu Sep 24 18:05:42 EEST 2015 i686
> Intel(R) Pentium(R) Dual CPU T3400 @ 2.16GHz GenuineIntel GNU/Linux
> 
> Gnu C                  4.8.5
> Gnu make               4.1
> binutils               2.25.1
> 1.1
> 2.25.1
> util-linux             /usr/src/linux/scripts/ver_linux.orig: line 23:
> fdformat: command not found
> mount                  debug
> module-init-tools      found
> Linux C Library        Dynamic linker (ldd)   2.20
> Procps                 3.3.9
> Net-tools              1.60_p20130513023548
> Kbd                    1.15.5
> Sh-utils               8.23
> Modules Loaded         xt_mark xt_LOG vgem v4l2_dv_timings ulpi udf
> sil164 nf_nat_sip nf_nat_irc uvcvideo videobuf2_vmalloc
> videobuf2_memops nf_nat_ipv4 videobuf2_core v4l2_common nf_nat_ftp
> nf_nat nf_log_ipv6 nf_log_ipv4 nf_log_common lcd i2c_mux hp_wireless
> gspca_main videodev cuse fuse crc_itu_t ch7006 ath5k coretemp
> input_leds led_class ath
> -------------------------------------
> Sample output of the patched implementation on Gentoo Linux:
> 
> If some fields are empty or look unusual you may have an old version.
> Compare to the current minimal requirements in Documentation/Changes.
> 
> Linux box1 4.0.5-gentoo #5 SMP Fri Sep 4 18:11:11 EEST 2015 i686
> Intel(R) Core(TM)2 Duo CPU E4600 @ 2.40GHz GenuineIntel GNU/Linux
> 
> GNU C            4.8.5
> GNU Make        4.1
> binutils        2.25.1
> util-linux        2.26.2
> mount            2.26.2
> module-init-tools    20
> e2fsprogs        1.42.13
> Linux C Library        2.20
> Dynamic linker (ldd)    2.20
> Linux C++ Library    6.0.19
> Procps            3.3.9
> net-tools        1.60
> Kbd            1.15.5
> Console-tools        1.15.5
> Sh-utils        8.23
> Modules Loaded        ch7006 coretemp crc_itu_t fuse gspca_main
> hwmon_vid i2c_mux it87 mii nf_log_common nf_log_ipv4 nf_log_ipv6
> nf_nat nf_nat_ftp nf_nat_ipv4 nf_nat_irc nf_nat_sip r8169 sil164
> snd_aloop udf uvcvideo v4l2_common v4l2_dv_timings videobuf2_core
> videobuf2_memops videobuf2_vmalloc videodev xt_LOG xt_mark
> -------------------------------------
> The actual patch.
> -------------------------------------

Why is this here?


> Signed-off-by: Alexander Kapshuk <alexander.kapshuk@...il.com>
> ---
> --- linux/scripts/ver_linux.orig    2015-04-13 01:12:50.000000000 +0300
> +++ linux/scripts/ver_linux    2015-09-29 20:47:48.664509177 +0300
> @@ -4,54 +4,112 @@
>  # /bin /sbin /usr/bin /usr/sbin /usr/local/bin, but it may
>  # differ on your system.
>  #
> +
>  echo 'If some fields are empty or look unusual you may have an old version.'
>  echo 'Compare to the current minimal requirements in Documentation/Changes.'
> -echo ' '
> +echo
> 
>  uname -a
> -echo ' '

You do a lot of different things in this patch, making it almost
impossible to review.  Please break them all up into individual changes,
each patch only doing one thing (like changing "echo ' '", cleaning up
argument lists, etc.) and resend this as a patch series that we can
review and hopefully apply to the tree.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists