[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