diff mbox series

tools/xenpvboot: remove as unable to convert to Python 3

Message ID 20231006145046.98450-1-roger.pau@citrix.com (mailing list archive)
State New, archived
Headers show
Series tools/xenpvboot: remove as unable to convert to Python 3 | expand

Commit Message

Roger Pau Monné Oct. 6, 2023, 2:50 p.m. UTC
The script heavily relies on the urlgrabber python module, which doesn't seem
to be packaged by all distros, it's missing from newer Debian versions at
least.

Also the usage of the commands module has been deprecated since Python 2.6, and
removed in Python 3, so the code would need to be re-written to not rely on
urlgrabber and the commands modules.

Arguably the purpose of the xenpvnetboot bootloader can also be achieved with
an isolated script that fetches the kernel and ramdisk before attempting to
launch the domain, without having to run in libxl context.  There are no xl.cfg
options consumed by the bootloader apart from the base location and the
subpaths of the kernel and ramdisk to fetch.

Any interested parties in keeping such functionality are free to submit an
updated script that works with Python 3.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 CHANGELOG.md            |   1 +
 tools/misc/Makefile     |   7 +-
 tools/misc/xenpvnetboot | 291 ----------------------------------------
 3 files changed, 2 insertions(+), 297 deletions(-)
 delete mode 100755 tools/misc/xenpvnetboot

Comments

Andrew Cooper Oct. 6, 2023, 2:55 p.m. UTC | #1
On 06/10/2023 3:50 pm, Roger Pau Monne wrote:
> The script heavily relies on the urlgrabber python module, which doesn't seem
> to be packaged by all distros, it's missing from newer Debian versions at
> least.
>
> Also the usage of the commands module has been deprecated since Python 2.6, and
> removed in Python 3, so the code would need to be re-written to not rely on
> urlgrabber and the commands modules.
>
> Arguably the purpose of the xenpvnetboot bootloader can also be achieved with
> an isolated script that fetches the kernel and ramdisk before attempting to
> launch the domain, without having to run in libxl context.  There are no xl.cfg
> options consumed by the bootloader apart from the base location and the
> subpaths of the kernel and ramdisk to fetch.
>
> Any interested parties in keeping such functionality are free to submit an
> updated script that works with Python 3.

Resolves: xen-project/xen#172

> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks for doing this.

~Andrew
Henry Wang Oct. 6, 2023, 2:57 p.m. UTC | #2
Hi,

> On Oct 6, 2023, at 22:55, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> On 06/10/2023 3:50 pm, Roger Pau Monne wrote:
>> The script heavily relies on the urlgrabber python module, which doesn't seem
>> to be packaged by all distros, it's missing from newer Debian versions at
>> least.
>> 
>> Also the usage of the commands module has been deprecated since Python 2.6, and
>> removed in Python 3, so the code would need to be re-written to not rely on
>> urlgrabber and the commands modules.
>> 
>> Arguably the purpose of the xenpvnetboot bootloader can also be achieved with
>> an isolated script that fetches the kernel and ramdisk before attempting to
>> launch the domain, without having to run in libxl context.  There are no xl.cfg
>> options consumed by the bootloader apart from the base location and the
>> subpaths of the kernel and ramdisk to fetch.
>> 
>> Any interested parties in keeping such functionality are free to submit an
>> updated script that works with Python 3.
> 
> Resolves: xen-project/xen#172
> 
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

> 
> Thanks for doing this.

+1

Kind regards,
Henry

> 
> ~Andrew
Andrew Cooper Oct. 6, 2023, 5:15 p.m. UTC | #3
On 06/10/2023 3:50 pm, Roger Pau Monne wrote:
> diff --git a/tools/misc/Makefile b/tools/misc/Makefile
> index 233a7948c050..9938bc235968 100644
> --- a/tools/misc/Makefile
> +++ b/tools/misc/Makefile
> @@ -36,11 +36,8 @@ INSTALL_SBIN                   += xen-livepatch
>  INSTALL_SBIN                   += xen-diag
>  INSTALL_SBIN += $(INSTALL_SBIN-y)
>  
> -# Everything to be installed in a private bin/
> -INSTALL_PRIVBIN                += xenpvnetboot
> -
>  # Everything to be installed
> -TARGETS_ALL := $(INSTALL_BIN) $(INSTALL_SBIN) $(INSTALL_PRIVBIN)
> +TARGETS_ALL := $(INSTALL_BIN) $(INSTALL_SBIN)
>  
>  # Everything which only needs copying to install
>  TARGETS_COPY += xencov_split

FYI.

Just out of context here is one more reference.  I've folded in the
following hunk.

diff --git a/tools/misc/Makefile b/tools/misc/Makefile
index 9938bc235968..66d0d6b09029 100644
--- a/tools/misc/Makefile
+++ b/tools/misc/Makefile
@@ -41,7 +41,6 @@ TARGETS_ALL := $(INSTALL_BIN) $(INSTALL_SBIN)
 
 # Everything which only needs copying to install
 TARGETS_COPY += xencov_split
-TARGETS_COPY += xenpvnetboot
 
 # Everything which needs to be built
 TARGETS := $(filter-out $(TARGETS_COPY),$(TARGETS_ALL))

~Andrew
Andrew Cooper Nov. 22, 2023, 8:30 p.m. UTC | #4
On 06/10/2023 3:50 pm, Roger Pau Monne wrote:
> The script heavily relies on the urlgrabber python module, which doesn't seem
> to be packaged by all distros, it's missing from newer Debian versions at
> least.
>
> Also the usage of the commands module has been deprecated since Python 2.6, and
> removed in Python 3, so the code would need to be re-written to not rely on
> urlgrabber and the commands modules.
>
> Arguably the purpose of the xenpvnetboot bootloader can also be achieved with
> an isolated script that fetches the kernel and ramdisk before attempting to
> launch the domain, without having to run in libxl context.  There are no xl.cfg
> options consumed by the bootloader apart from the base location and the
> subpaths of the kernel and ramdisk to fetch.
>
> Any interested parties in keeping such functionality are free to submit an
> updated script that works with Python 3.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

I've just found
https://build.opensuse.org/package/view_file/openSUSE:Leap:15.5:Update/xen/bin-python3-conversion.patch?expand=1

Which includes the modifications to make this work with Python 3.

Does this mean there are SLES/OpenSUSE users of xenpvboot ?

~Andrew
Olaf Hering Nov. 22, 2023, 8:43 p.m. UTC | #5
Wed, 22 Nov 2023 20:30:59 +0000 Andrew Cooper <andrew.cooper3@citrix.com>:

> Does this mean there are SLES/OpenSUSE users of xenpvboot ?

We do not know. It is gone by now, in 4.18:

https://build.opensuse.org/request/show/1126897


Olaf
Andrew Cooper Nov. 22, 2023, 8:46 p.m. UTC | #6
On 22/11/2023 8:43 pm, Olaf Hering wrote:
> Wed, 22 Nov 2023 20:30:59 +0000 Andrew Cooper <andrew.cooper3@citrix.com>:
>
>> Does this mean there are SLES/OpenSUSE users of xenpvboot ?
> We do not know. It is gone by now, in 4.18:
>
> https://build.opensuse.org/request/show/1126897

Yes.  The email I replied to was the patch we merged into 4.18 deleting
it because we thought noone had ever made it function for Py3, with the
implication being that if this was going to be a problem, we could
reinstate it with the fixes from your patch.

But that entirely depends on whether you think anyone is using it or not.

~Andrew
Olaf Hering Nov. 22, 2023, 9:27 p.m. UTC | #7
Wed, 22 Nov 2023 20:46:15 +0000 Andrew Cooper <andrew.cooper3@citrix.com>:

> But that entirely depends on whether you think anyone is using it or not.

We never got any bugreport, nor have we any indication for actual usage.
The script was in the sources. Python3 is supposed to be the default
interpreter since a number of years, so something had to be done with
every installed python script in the sources.

I think if anyone shows up and demands this script, it could be restored.
Otherwise it is one less thing to worry about.


Olaf
Andrew Cooper Nov. 22, 2023, 9:29 p.m. UTC | #8
On 22/11/2023 9:27 pm, Olaf Hering wrote:
> Wed, 22 Nov 2023 20:46:15 +0000 Andrew Cooper <andrew.cooper3@citrix.com>:
>
>> But that entirely depends on whether you think anyone is using it or not.
> We never got any bugreport, nor have we any indication for actual usage.
> The script was in the sources. Python3 is supposed to be the default
> interpreter since a number of years, so something had to be done with
> every installed python script in the sources.
>
> I think if anyone shows up and demands this script, it could be restored.
> Otherwise it is one less thing to worry about.

Ok.  Lets leave it to rest as it is.

If we need to resurrect it, there's enough context on this thread to get
a Py3 compatible one.

~Andrew
diff mbox series

Patch

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 47ea9e275462..165c5caf9bea 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -38,6 +38,7 @@  The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
  - On x86, the "pku" command line option has been removed.  It has never
    behaved precisely as described, and was redundant with the unsupported
    "cpuid=no-pku".  Visibility of PKU to guests should be via its vm.cfg file.
+ - xenpvnetboot removed as unable to convert to Python 3.
 
 ## [4.17.0](https://xenbits.xen.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.17.0) - 2022-12-12
 
diff --git a/tools/misc/Makefile b/tools/misc/Makefile
index 233a7948c050..9938bc235968 100644
--- a/tools/misc/Makefile
+++ b/tools/misc/Makefile
@@ -36,11 +36,8 @@  INSTALL_SBIN                   += xen-livepatch
 INSTALL_SBIN                   += xen-diag
 INSTALL_SBIN += $(INSTALL_SBIN-y)
 
-# Everything to be installed in a private bin/
-INSTALL_PRIVBIN                += xenpvnetboot
-
 # Everything to be installed
-TARGETS_ALL := $(INSTALL_BIN) $(INSTALL_SBIN) $(INSTALL_PRIVBIN)
+TARGETS_ALL := $(INSTALL_BIN) $(INSTALL_SBIN)
 
 # Everything which only needs copying to install
 TARGETS_COPY += xencov_split
@@ -59,11 +56,9 @@  install: all
 	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
 	$(INSTALL_PYTHON_PROG) $(INSTALL_BIN) $(DESTDIR)$(bindir)
 	$(INSTALL_PYTHON_PROG) $(INSTALL_SBIN) $(DESTDIR)$(sbindir)
-	$(INSTALL_PYTHON_PROG) $(INSTALL_PRIVBIN) $(DESTDIR)$(LIBEXEC_BIN)
 
 .PHONY: uninstall
 uninstall:
-	rm -f $(addprefix $(DESTDIR)$(LIBEXEC_BIN)/, $(INSTALL_PRIVBIN))
 	rm -f $(addprefix $(DESTDIR)$(sbindir)/, $(INSTALL_SBIN))
 	rm -f $(addprefix $(DESTDIR)$(bindir)/, $(INSTALL_BIN))
 
diff --git a/tools/misc/xenpvnetboot b/tools/misc/xenpvnetboot
deleted file mode 100755
index be972b9e19b7..000000000000
--- a/tools/misc/xenpvnetboot
+++ /dev/null
@@ -1,291 +0,0 @@ 
-#!/usr/bin/env python
-#
-# Copyright (C) 2010 Oracle. All rights reserved.
-#
-# This program is free software; you can redistribute it and/or modify it under
-# the terms of the GNU General Public License as published by the Free Software
-# Foundation, version 2.  This program is distributed in the hope that it will be
-# useful, but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General
-# Public License for more details.  You should have received a copy of the GNU
-# General Public License along with this program; If not, see <http://www.gnu.org/licenses/>.
-
-import sys
-import os
-import stat
-import time
-import string
-import random
-import tempfile
-import commands
-import subprocess
-import urlgrabber
-from optparse import OptionParser
-
-
-XEN_PATHS = [
-    ('images/xen/vmlinuz', 'images/xen/initrd.img'), # Fedora <= 10 and OL = 5
-    ('boot/i386/vmlinuz-xen', 'boot/i386/initrd-xen'), # openSUSE >= 10.2 and SLES >= 10
-    ('boot/x86_64/vmlinuz-xen', 'boot/x86_64/initrd-xen'), # openSUSE >= 10.2 and SLES >= 10
-    ('current/images/netboot/xen/vmlinuz', 'current/images/netboot/xen/initrd.gz'), # Debian
-    ('images/pxeboot/vmlinuz', 'images/pxeboot/initrd.img'), # Fedora >=10 and OL >= 6
-    ('isolinux/vmlinuz', 'isolinux/initrd.img'), # Fedora >= 10 and OL >= 6
-]
-
-
-def format_sxp(kernel, ramdisk, args):
-    s = 'linux (kernel %s)' % kernel
-    if ramdisk:
-        s += '(ramdisk %s)' % ramdisk
-    if args:
-        s += '(args "%s")' % args
-    return s
-
-
-def format_simple(kernel, ramdisk, args, sep):
-    s = ('kernel %s' % kernel) + sep
-    if ramdisk:
-        s += ('ramdisk %s' % ramdisk) + sep
-    if args:
-        s += ('args %s' % args) + sep
-    s += sep
-    return s
-
-
-def mount(dev, path, option=''):
-    if os.uname()[0] == 'SunOS':
-        mountcmd = '/usr/sbin/mount'
-    else:
-        mountcmd = '/bin/mount'
-    cmd = ' '.join([mountcmd, option, dev, path])
-    (status, output) = commands.getstatusoutput(cmd)
-    if status != 0:
-        raise RuntimeError('Command: (%s) failed: (%s) %s' % (cmd, status, output))
-
-
-def umount(path):
-    if os.uname()[0] == 'SunOS':
-        cmd = ['/usr/sbin/umount', path]
-    else:
-        cmd = ['/bin/umount', path]
-    subprocess.call(cmd)
-
-
-class Fetcher:
-    def __init__(self, location, tmpdir):
-        self.location = location
-        self.tmpdir = tmpdir
-        self.srcdir = location
-
-    def prepare(self):
-        if not os.path.exists(self.tmpdir):
-            os.makedirs(self.tmpdir, 0750)
-
-    def cleanup(self):
-        pass
-
-    def get_file(self, filename):
-        url = os.path.join(self.srcdir, filename)
-        suffix = ''.join(random.sample(string.ascii_letters, 6))
-        local_name = os.path.join(self.tmpdir, 'xenpvboot.%s.%s' % (os.path.basename(filename), suffix))
-        try:
-            return urlgrabber.urlgrab(url, local_name, copy_local=1)
-        except Exception, err:
-            raise RuntimeError('Cannot get file %s: %s' % (url, err))
-
-
-class MountedFetcher(Fetcher):
-    def prepare(self):
-        Fetcher.prepare(self)
-        self.srcdir = tempfile.mkdtemp(prefix='xenpvboot.', dir=self.tmpdir)
-        if self.location.startswith('nfs:'):
-            mount(self.location[4:], self.srcdir, '-o ro')
-        else:
-            if stat.S_ISBLK(os.stat(self.location)[stat.ST_MODE]):
-                option = '-o ro'
-            else:
-                option = '-o ro,loop'
-            if os.uname()[0] == 'SunOS':
-                option += ' -F hsfs'
-            mount(self.location, self.srcdir, option)
-
-    def cleanup(self):
-        umount(self.srcdir)
-        try:
-            os.rmdir(self.srcdir)
-        except:
-            pass
-
-
-class NFSISOFetcher(MountedFetcher):
-    def __init__(self, location, tmpdir):
-        self.nfsdir = None
-        MountedFetcher.__init__(self, location, tmpdir)
-
-    def prepare(self):
-        Fetcher.prepare(self)
-        self.nfsdir = tempfile.mkdtemp(prefix='xenpvboot.', dir=self.tmpdir)
-        self.srcdir = tempfile.mkdtemp(prefix='xenpvboot.', dir=self.tmpdir)
-        nfs = os.path.dirname(self.location[8:])
-        iso = os.path.basename(self.location[8:])
-        mount(nfs, self.nfsdir, '-o ro')
-        option = '-o ro,loop'
-        if os.uname()[0] == 'SunOS':
-            option += ' -F hsfs'
-        mount(os.path.join(self.nfsdir, iso), self.srcdir, option)
-
-    def cleanup(self):
-        MountedFetcher.cleanup(self)
-        time.sleep(1)
-        umount(self.nfsdir)
-        try:
-            os.rmdir(self.nfsdir)
-        except:
-            pass
-
-
-class TFTPFetcher(Fetcher):
-    def get_file(self, filename):
-        if '/' in self.location[7:]:
-            host = self.location[7:].split('/', 1)[0].replace(':', ' ')
-            basedir = self.location[7:].split('/', 1)[1]
-        else:
-            host = self.location[7:].replace(':', ' ')
-            basedir = ''
-        suffix = ''.join(random.sample(string.ascii_letters, 6))
-        local_name = os.path.join(self.tmpdir, 'xenpvboot.%s.%s' % (os.path.basename(filename), suffix))
-        cmd = '/usr/bin/tftp %s -c get %s %s' % (host, os.path.join(basedir, filename), local_name)
-        (status, output) = commands.getstatusoutput(cmd)
-        if status != 0:
-            raise RuntimeError('Command: (%s) failed: (%s) %s' % (cmd, status, output))
-        return local_name
-
-
-def main():
-    usage = '''%prog [option]
-
-Get boot images from the given location and prepare for Xen to use.
-
-Supported locations:
-
- - http://host/path
- - https://host/path
- - ftp://host/path
- - file:///path
- - tftp://host/path
- - nfs:host:/path
- - /path
- - /path/file.iso
- - /path/filesystem.img
- - /dev/sda1
- - nfs+iso:host:/path/file.iso
- - nfs+iso:host:/path/filesystem.img'''
-    version = '%prog version 0.1'
-    parser = OptionParser(usage=usage, version=version)
-    parser.add_option('', '--location',
-                      help='The base url for kernel and ramdisk files.')
-    parser.add_option('', '--kernel',
-                      help='The kernel image file.')
-    parser.add_option('', '--ramdisk',
-                      help='The initial ramdisk file.')
-    parser.add_option('', '--args',
-                      help='Arguments pass to the kernel.')
-    parser.add_option('', '--output',
-                      help='Redirect output to this file instead of stdout.')
-    parser.add_option('', '--output-directory', default='/var/run/libxl',
-                      help='Output directory.')
-    parser.add_option('', '--output-format', default='sxp',
-                      help='Output format: sxp, simple or simple0.')
-    parser.add_option('-q', '--quiet', action='store_true',
-                      help='Be quiet.')
-    (opts, args) = parser.parse_args()
-
-    if not opts.location and not opts.kernel and not opts.ramdisk:
-        if not opts.quiet:
-            print >> sys.stderr, 'You should at least specify a location or kernel/ramdisk.'
-            parser.print_help(sys.stderr)
-        sys.exit(1)
-
-    if not opts.output or opts.output == '-':
-        fd = sys.stdout.fileno()
-    else:
-        fd = os.open(opts.output, os.O_WRONLY)
-
-    if opts.location:
-        location = opts.location
-    else:
-        location = ''
-    if (location == ''
-        or location.startswith('http://') or location.startswith('https://')
-        or location.startswith('ftp://') or location.startswith('file://')
-        or (os.path.exists(location) and os.path.isdir(location))):
-        fetcher = Fetcher(location, opts.output_directory)
-    elif location.startswith('nfs:') or (os.path.exists(location) and not os.path.isdir(location)):
-        fetcher = MountedFetcher(location, opts.output_directory)
-    elif location.startswith('nfs+iso:'):
-        fetcher = NFSISOFetcher(location, opts.output_directory)
-    elif location.startswith('tftp://'):
-        fetcher = TFTPFetcher(location, opts.output_directory)
-    else:
-        if not opts.quiet:
-            print >> sys.stderr, 'Unsupported location: %s' % location
-        sys.exit(1)
-
-    try:
-        fetcher.prepare()
-    except Exception, err:
-        if not opts.quiet:
-            print >> sys.stderr, str(err)
-        fetcher.cleanup()
-        sys.exit(1)
-
-    try:
-        kernel = None
-        if opts.kernel:
-            kernel = fetcher.get_file(opts.kernel)
-        else:
-            for (kernel_path, _) in XEN_PATHS:
-                try:
-                    kernel = fetcher.get_file(kernel_path)
-                except Exception, err:
-                    if not opts.quiet:
-                        print >> sys.stderr, str(err)
-                    continue
-                break
-
-        if not kernel:
-            if not opts.quiet:
-                print >> sys.stderr, 'Cannot get kernel from loacation: %s' % location
-            sys.exit(1)
-
-        ramdisk = None
-        if opts.ramdisk:
-            ramdisk = fetcher.get_file(opts.ramdisk)
-        else:
-            for (_, ramdisk_path) in XEN_PATHS:
-                try:
-                    ramdisk = fetcher.get_file(ramdisk_path)
-                except Exception, err:
-                    if not opts.quiet:
-                        print >> sys.stderr, str(err)
-                    continue
-                break
-    finally:
-        fetcher.cleanup()
-
-    if opts.output_format == 'sxp':
-        output = format_sxp(kernel, ramdisk, opts.args)
-    elif opts.output_format == 'simple':
-        output = format_simple(kernel, ramdisk, opts.args, '\n')
-    elif opts.output_format == 'simple0':
-        output = format_simple(kernel, ramdisk, opts.args, '\0')
-    else:
-        print >> sys.stderr, 'Unknown output format: %s' % opts.output_format
-        sys.exit(1)
-
-    sys.stdout.flush()
-    os.write(fd, output)
-
-
-if __name__ == '__main__':
-    main()