[libblockdev 2/3] Add the s390 plugin functions.

Vratislav Podzimek vpodzime at redhat.com
Fri May 15 07:55:04 UTC 2015


On Wed, 2015-05-13 at 14:16 -0400, Samantha N. Bueno wrote:
> This includes the header, api, and source for all s390-related
> functions, as well as the Python bindings.
> ---
>  dist/libblockdev.spec               |  32 +++
>  src/lib/plugin_apis/s390.api        |  78 +++++++
>  src/plugins/s390.c                  | 421 ++++++++++++++++++++++++++++++++++++
>  src/plugins/s390.h                  |  79 +++++++
>  src/python/gi/overrides/BlockDev.py |  50 +++++
>  5 files changed, 660 insertions(+)
>  create mode 100644 src/lib/plugin_apis/s390.api
>  create mode 100644 src/plugins/s390.c
>  create mode 100644 src/plugins/s390.h
> 
> diff --git a/dist/libblockdev.spec b/dist/libblockdev.spec
> index 47f1c47..73f2268 100644
> --- a/dist/libblockdev.spec
> +++ b/dist/libblockdev.spec
> @@ -217,6 +217,25 @@ Requires: glib2-devel
>  This package contains header files and pkg-config files needed for development
>  with the libblockdev-swap plugin/library.
>  
> +%package s390
> +Summary:    The s390 plugin for the libblockdev library
> +Requires: s390utils
> +
> +%description s390
> +The libblockdev library plugin (and in the same time a standalone library)
> +providing the functionality related to s390 devices.
> +
> +%package s390-devel
> +Summary:     Development files for the libblockdev-s390 plugin/library
> +Requires: %{name}-s390%{?_isa} = %{version}-%{release}
> +Requires: %{name}-utils-devel%{?_isa}
> +Requires: glib2-devel
> +Requires: s390utils-devel
> +
> +%description s390-devel
> +This package contains header files and pkg-config files needed for development
> +with the libblockdev-s390 plugin/library.
> +
>  
>  %package plugins-all
>  Summary:     Meta-package that pulls all the libblockdev plugins as dependencies
> @@ -229,6 +248,9 @@ Requires: %{name}-lvm%{?_isa} = %{version}-%{release}
>  Requires: %{name}-mdraid%{?_isa} = %{version}-%{release}
>  Requires: %{name}-mpath%{?_isa} = %{version}-%{release}
>  Requires: %{name}-swap%{?_isa} = %{version}-%{release}
> +%ifarch s390 s390x
> +Requires: %{name}-s390%{?_isa} = %{version}-%{release}
> +%endif
>  
>  %description plugins-all
>  A meta-package that pulls all the libblockdev plugins as dependencies.
> @@ -264,6 +286,8 @@ CFLAGS="%{optflags}" make PREFIX=%{buildroot} SITEDIRS=%{buildroot}%{python2_sit
>  %postun mpath -p /sbin/ldconfig
>  %post swap -p /sbin/ldconfig
>  %postun swap -p /sbin/ldconfig
> +%post s390 -p /sbin/ldconfig
> +%postun s390 -p /sbin/ldconfig
>  
> 
>  %files
> @@ -368,6 +392,14 @@ CFLAGS="%{optflags}" make PREFIX=%{buildroot} SITEDIRS=%{buildroot}%{python2_sit
>  %dir %{_includedir}/blockdev
>  %{_includedir}/blockdev/swap.h
>  
> +%files s390
> +%{_libdir}/libbd_s390.so.*
> +
> +%files s390-devel
> +%{_libdir}/libbd_s390.so
> +%dir %{_includedir}/blockdev
> +%{_includedir}/blockdev/s390.h
> +
>  %files plugins-all
>  
>  %changelog
> diff --git a/src/lib/plugin_apis/s390.api b/src/lib/plugin_apis/s390.api
> new file mode 100644
> index 0000000..54984fe
> --- /dev/null
> +++ b/src/lib/plugin_apis/s390.api
> @@ -0,0 +1,78 @@
> +#include <glib.h>
> +#include <linux/fs.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <utils.h>
> +#include <asm/dasd.h>
> +#include <s390utils/vtoc.h>
> +
> +
> +typedef enum {
> +    BD_S390_ERROR_DEVICE,
> +    BD_S390_ERROR_FORMAT_FAILED,
> +    BD_S390_ERROR_PARSE,
> +} BDS390Error;
> +
> +
> +/**
> + * bd_s390_dasd_format:
> + * @dasd: dasd to format
> + * @error: (out): place to store error (if any)
> + *
> + * Returns: whether dasdfmt was successful or not
> + */
> +gboolean bd_s390_dasd_format (gchar *dasd, GError **error);
> +
> +/**
> + * bd_s390_dasd_needs_format:
> + * @dasd: dasd to check, whether it needs dasdfmt run on it
> + * @error: (out): place to store error (if any)
> + *
> + * Returns: whether a dasd needs dasdfmt run against it
> + */
> +gboolean bd_s390_dasd_needs_format (gchar *dasd, GError **error);
> +
> +/**
> + * bd_s390_dasd_online:
> + * @dasd: dasd to switch online
> + * @error: (out): place to store error (if any)
> + *
> + * Returns: whether a dasd was successfully switched online
> + */
> +gboolean bd_s390_dasd_online (gchar *dasd, GError **error);
> +
> +/**
> + * bd_s390_dasd_is_Ldl:
> + * @dasd: dasd to check, whether it is LDL formatted
> + * @error: (out): place to store error (if any)
> + *
> + * Returns: whether a dasd is LDL formatted
> + */
> +gboolean bd_s390_dasd_is_ldl (gchar *dasd, GError **error);
> +
> +/**
> + * bd_s390_sanitize_dev_input:
> + * @dev a DASD or zFCP device number
> + * @error: (out): place to store error (if any)
> + *
> + * Returns: a synthesized dasd or zfcp device number
> + */
> +gchar* bd_s390_sanitize_dev_input (gchar *dev, GError **error);
> +
> +/**
> + * bd_s390_zfcp_sanitize_wwpn_input:
> + * @dev a zFCP WWPN identifier
> + * @error: (out): place to store error (if any)
> + *
> + * Returns: a synthesized zFCP WWPN
> + */
> +gchar* bd_s390_zfcp_sanitize_wwpn_input (gchar *wwpn, GError **error);
> +
> +/**
> + * bd_s390_zfcp_sanitize_lun_input:
> + * @dev a zFCP LUN identifier
> + * @error: (out): place to store error (if any)
> + *
> + * Returns: a synthesized zFCP LUN
> + */
> +gchar* bd_s390_zfcp_sanitize_lun_input (gchar *lun, GError **error);
> diff --git a/src/plugins/s390.c b/src/plugins/s390.c
> new file mode 100644
> index 0000000..4167ac7
> --- /dev/null
> +++ b/src/plugins/s390.c
> @@ -0,0 +1,421 @@
> +/*
> + * Copyright (C) 2015  Red Hat, Inc.
> + *
> + * 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; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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/>.
> + *
> + * Author: Samantha N. Bueno <sbueno at redhat.com>
> + */
> +
> +#include <glib.h>
> +#include <linux/fs.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <utils.h>
> +#include <asm/dasd.h>
> +#include <s390utils/vtoc.h>
> +
> +#include "s390.h"
> +
> +
> +/**
> + *  * SECTION: s390
> + *   * @short_description: plugin for operations with s390
> + *    * @title: s390
> + *     * @include: s390.h
> + *      *
> + *       * A plugin for operations with s390 devices.
> + *         */
Any reason for this growing number of spaces?

> +
> +/**
> + *  * bd_s390_error_quark: (skip)
> + *   */
> +GQuark bd_s390_error_quark (void) {
> +    return g_quark_from_static_string ("g-bd-s390-error-quark");
> +}
> +
> +
> +/**
> + * check: (skip)
> + */
> +gboolean check() {
> +    GError *error = NULL;
> +    /* dasdfmt doesn't return version info */
> +    gboolean ret = bd_utils_check_util_version ("dasdfmt", NULL, NULL, NULL, &error);
> +    if (!ret && error) {
> +        g_warning("Cannot load the s390 plugin: %s" , error->message);
> +        g_clear_error (&error);
> +    }
> +    return ret;
> +}
> +
> +/**
> + * bd_s390_dasd_format:
> + * @dasd: dasd to format
> + * @error: (out): place to store error (if any)
> + *
> + * Returns: whether dasdfmt was successful or not
> + */
> +gboolean bd_s390_dasd_format (gchar *dasd, GError **error) {
> +    gint rc = 0;
> +    gchar *cmd = NULL;
> +    gchar *failed = NULL;
> +
> +    cmd = g_strdup_printf ("/sbin/dasdfmt -y -d cdl -b 4096 /dev/%s", dasd);
> +    rc = system (cmd);
> +    g_free (cmd);
> +
> +    /* rc is 0 on successful dasdfmt */
> +    if (rc) {
> +        failed = g_strdup_printf ("dasdfmt failed: %d", rc);
> +        g_set_error (error, BD_S390_ERROR, BD_S390_ERROR_FORMAT_FAILED, failed);
> +        g_free (failed);
> +        return FALSE;
> +    }
As I already told Samantha on IRC, bd_utils_exec_and_report_error()
should be used instead of system() as it takes care of logging and some
other things.

> +
> +    return TRUE;
> +}
> +
> +/**
> + * bd_s390_dasd_needs_format:
> + * @dasd: dasd to check, given as device number
> + * @error: (out): place to store error (if any)
> + *
> + * Returns: whether a dasd needs dasdfmt run against it
> + */
> +gboolean bd_s390_dasd_needs_format (gchar *dasd, GError **error) {
> +    gchar *status = NULL;
> +    gchar *path = NULL;
> +    gchar *rc = NULL;
> +    FILE *fd = NULL;
> +
> +    path = g_strdup_printf ("/sys/bus/ccw/drivers/dasd-eckd/%s/status", dasd);
> +    fd = fopen(path, "r");
> +    if (!fd)
> +        g_set_error (error, BD_S390_ERROR, BD_S390_ERROR_DEVICE, "Error checking status of device %s; device may not exist, or status can not be read.", dasd);
> +        g_clear_error (error);
> +        return FALSE;
This cannot work, {}s are needed around the code block, otherwise this
always clears the error and returns FALSE. Also 'path' should be free'd
in the block above or, preferably, right after the 'fd =' assignment
instead of after the 'rc =' assignment below.

> +
> +    rc = fgets (status, 11, fd);
> +    g_free (path);
If you close the 'fd' here, you won't have to close it in three
different places below.

> +
> +    if (!rc) {
> +        g_set_error (error, BD_S390_ERROR, BD_S390_ERROR_DEVICE, "Error checking status of device %s.", dasd);
> +        g_clear_error (error);
> +        fclose(fd);
> +        return FALSE;
> +    }
> +
> +    if (g_ascii_strncasecmp (status, "unformatted", 11) == 0) {
the fgets() above reads at most 10 characters (see 'man fgets') and you
compare 11 chars here. 

> +        g_warning ("%s (/dev/%s) status is %s, needs dasdfmt.", dasd, dasd, status);
> +        g_clear_error (error);
> +        fclose(fd);
> +        return TRUE;
> +    }
> +
> +    fclose(fd);
> +    return FALSE;
> +}
> +
> +/**
> + * bd_s390_dasd_online:
> + * @dasd: dasd to switch online, given as device number
> + * @error: (out): place to store error (if any)
> + *
> + * Returns: whether a dasd was successfully switched online
> + */
> +gboolean bd_s390_dasd_online (gchar *dasd, GError **error) {
> +    gboolean rc = FALSE;
> +    gint wrc = 0;
> +    gint online = 0;
> +    gchar *path = NULL;
> +    gchar *cmd = NULL;
> +    FILE *fd = NULL;
> +
> +    path = g_strdup_printf ("/sys/bus/ccw/drivers/dasd-eckd/%s/online", dasd);
> +    fd = fopen(path, "r+");
Should free 'path' here.

> +    if (!fd) {
> +        /* DASD might be in device ignore list; try to rm it */
> +        cmd = g_strdup_printf ("/usr/sbin/dasd_cio_free -d %s", dasd);
> +        rc = system (cmd);
> +        g_free(cmd);
> +
> +        if (!rc) {
> +            g_set_error (error, BD_S390_ERROR, BD_S390_ERROR_DEVICE, "Could not free device %s.", dasd);
> +            g_clear_error (error);
> +            fclose(fd);
> +            return FALSE;
> +        }
> +        /* fd is NULL at this point, so try to open it */
> +        fd = fopen(path, "r+");
> +        g_free (path);
> +
> +        if (!fd) {
> +            g_set_error (error, BD_S390_ERROR, BD_S390_ERROR_DEVICE, "Could not open device %s even after removing it from the device ignore list.", dasd);
> +            g_clear_error (error);
> +            fclose(fd);
> +            return FALSE;
> +        }
> +    }
> +
> +    /* check whether our DASD is online; if not, set it */
> +    online = fgetc(fd);
> +
> +    if (online == EOF) {
> +        /* there was some error checking the 'online' status */
> +        g_set_error (error, BD_S390_ERROR, BD_S390_ERROR_DEVICE, "Error checking if device %s is online", dasd);
> +        g_clear_error (error);
> +        fclose(fd);
> +        return FALSE;
> +    }
> +    if (online == 1) {
> +        g_set_error (error, BD_S390_ERROR, BD_S390_ERROR_DEVICE, "DASD device %s is already online.", dasd);
> +        g_clear_error (error);
> +        fclose(fd);
> +        return FALSE;
> +    }
> +    else {
> +        wrc = fputc(1, fd);
Are you sure this writes the 1 to the beginning of the file and not on
the second position after the first int that was read above?

> +    }
> +
> +    fclose(fd);
> +
> +    if (wrc == EOF) {
> +        g_warning ("Could not set DASD device %s online", dasd);
> +        fclose(fd);
> +        g_clear_error (error);
> +        return FALSE;
> +    }
> +
> +    return TRUE;
> +}
> +
> +/**
> + * bd_s390_dasd_read_vlabel:
> + * @dasd_info:
> + * @fd: a file descriptor for the open DASD device
> + * @blksize:
> + * @vlabel:
> + *
> + * Returns:
> + */
> +static int bd_s390_dasd_read_vlabel(dasd_information2_t *dasd_info, int fd, int blksize, volume_label_t *vlabel) {
You don't have to use the 'bd_s390' prefix for internal (static)
functions.

> +    gint rc;
> +    unsigned long vlabel_start;
> +    vlabel_start = dasd_info->label_block * blksize;
> +
> +    memset(vlabel, 0, sizeof(volume_label_t));
> +
> +    if (lseek(fd, vlabel_start, SEEK_SET) < 0) {
> +        return 2;
> +    }
> +
> +    rc = read(fd, vlabel, sizeof(volume_label_t));
> +    if (rc != sizeof(volume_label_t)) {
> +        return 1;
> +    }
> +
> +    return 0;
> +}
This is where I ended the review before going to Brno. :)

-- 
Vratislav Podzimek

Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic



More information about the anaconda-patches mailing list