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

David Shea dshea at redhat.com
Wed May 20 12:51:59 UTC 2015


On 05/19/2015 10:30 PM, 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                  | 394 ++++++++++++++++++++++++++++++++++++
>   src/plugins/s390.h                  |  24 +++
>   src/python/gi/overrides/BlockDev.py |  43 ++++
>   5 files changed, 571 insertions(+)
>   create mode 100644 src/lib/plugin_apis/s390.api
>   create mode 100644 src/plugins/s390.c
>   create mode 100644 src/plugins/s390.h

In all of the new API functions, please use "const gchar *" instead of 
"gchar *". All but one of the parameters are duplicated within the 
function (either through g_strdup_printf or g_ascii_strdown), so you 
don't need to change anything else. The exception is in 
bd_s390_dasd_online, where the parameter is added to a gchar * array and 
passed to bd_utils_exec_and_report_error. There was some discussion on 
IRC about that and a different glib API that might allow the parameter 
list to be const was mentioned, so for now I guess just leave that one 
not-const and add an explicit (transfer none) to the parameter in the 
doc comment.

Using consts makes it play a lot nicer with gobject-introspection.

> diff --git a/src/plugins/s390.c b/src/plugins/s390.c
> new file mode 100644
> index 0000000..6d663b8
> --- /dev/null
> +++ b/src/plugins/s390.c
> @@ -0,0 +1,394 @@
> +/*
> + * 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.
> + */
> +
> +/**
> + * 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) {
> +    gboolean rc = FALSE;
> +    gchar *dev = g_strdup_printf ("/dev/%s", dasd);
> +    gchar *argv[] = {"/sbin/dasdfmt", "-y", "-d", "cdl", "-b", "4096", dev, NULL};
> +
> +    rc = bd_utils_exec_and_report_error (argv, error);
> +    g_free (dev);
> +    return rc;
> +}
> +
> +/**
> + * 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[12];
> +    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");
> +    g_free (path);
> +    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;
> +    }
> +
> +    /* read 'status' value; will either be 'unformatted' or 'online' */
> +    memset (status, 0, sizeof(status));
> +    rc = fgets (status, 12, fd);
> +    fclose(fd);
> +
> +    if (!rc) {
> +        g_set_error (error, BD_S390_ERROR, BD_S390_ERROR_DEVICE,
> +                     "Error checking status of device %s.", dasd);
> +        g_clear_error (error);
> +        return FALSE;
> +    }
> +
> +    if (g_ascii_strncasecmp (status, "unformatted", strlen(status)) == 0) {
> +        g_warning ("Device %s status is %s, needs dasdfmt.", dasd, status);
> +        g_clear_error (error);
> +        return TRUE;
> +    }
> +
> +    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;
> +    FILE *fd = NULL;
> +    gchar *argv[] = {"/usr/sbin/dasd_cio_free", "-d", dasd, NULL};
> +
> +    path = g_strdup_printf ("/sys/bus/ccw/drivers/dasd-eckd/%s/online", dasd);
> +    fd = fopen(path, "r+");
> +    if (!fd) {
> +        /* DASD might be in device ignore list; try to rm it */
> +        rc = bd_utils_exec_and_report_error (argv, error);
> +
> +        if (!rc) {
> +            return rc;
> +        }

This is a potential memory leak, since path is still allocated at the 
"return rc" line here.

> +        /* 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);
> +            return FALSE;
> +        }
> +    }
> +    else {
> +        g_free (path);
> +    }
> +
> +    /* 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);
> +        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);
> +        fclose(fd);
> +        return FALSE;
> +    }
> +    else {
> +        /* reset file cursor before writing to it */
> +        rewind (fd);
> +        wrc = fputs("1", fd);
> +    }
> +
> +    fclose(fd);
> +
> +    if (wrc == EOF) {
> +        g_warning ("Could not set DASD device %s online", dasd);
> +        fclose(fd);
> +        return FALSE;
> +    }
> +
> +    return TRUE;
> +}
> +
> +/**
> + * 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) {
> +    gchar *devname = NULL;
> +    gint f = 0;
> +    gint blksize = 0;
> +    dasd_information2_t dasd_info;
> +
> +    memset(&dasd_info, 0, sizeof(dasd_info));
> +    devname = g_strdup_printf ("/dev/%s", dasd);
> +
> +    if ((f = open(devname, O_RDONLY)) == -1) {
> +        g_set_error (error, BD_S390_ERROR, BD_S390_ERROR_DEVICE,
> +                     "Unable to open device %s", devname);
> +        g_free (devname);
> +        return FALSE;
> +    }

This is the only place where devname is freed, so it will leak memory if 
the open actually works.

> +    if (ioctl(f, BLKSSZGET, &blksize) != 0) {
> +        close(f);
> +        return FALSE;
> +    }
> +
> +    if (ioctl(f, BIODASDINFO2, &dasd_info) != 0) {
> +        close(f);
> +        return FALSE;
> +    }
> +
> +    close(f);
> +
> +    /* check we're not on an FBA DASD, since dasdfmt can't run on them */
> +    if (strncmp(dasd_info.type, "FBA", 3) == 0) {
> +        return FALSE;
> +    }
> +
> +    /* check dasd volume label; "VOL1" is a CDL formatted DASD, won't require formatting */
> +    if (dasd_info.format == DASD_FORMAT_CDL) {
> +        return FALSE;
> +    }
> +    else {
> +        return TRUE;
> +    }
> +}
> +
> +/**
> + * bd_s390_sanitize_dev_input:
> + * @dev a DASD or zFCP device number

Missing a colon after @dev in the comment block.

> + * @error: (out): place to store error (if any)
> + *
> + * Returns: (transfer full): a synthesized dasd or zfcp device number

It would be nice to mention in the doc comment that the return value 
needs to be freed by the caller. The "transfer full" tells that to 
gobject-introspection, but sometimes humans need human words.

> + */
> +gchar* bd_s390_sanitize_dev_input (gchar *dev, GError **error) {
> +    gchar *tok = NULL;
> +    gchar *tmptok = NULL;
> +    gchar *prepend = NULL;
> +    gchar *ret = NULL;
> +    gchar *lcdev = NULL;
> +
> +    /* first make sure we're not being played */
> +    if ((dev == NULL) || (!*dev)) {
> +        g_set_error (error, BD_S390_ERROR, BD_S390_ERROR_DEVICE,
> +                     "You have not specified a device number or the number is invalid");
> +        g_clear_error (error);
> +        return "";
> +    }
> +
> +    /* convert everything to lowercase */
> +    lcdev = g_ascii_strdown (dev, -1);
> +
> +    /* we only care about the last piece of the device number, since
> +     * that's the only part which will need formatting */
> +    tok = g_strrstr(lcdev, ".");
> +
> +    if (tok) {
> +        tmptok = tok + 1; /* want to ignore the delimiter char */
> +    }
> +    else {
> +        tmptok = lcdev;
> +    }
> +
> +    /* calculate if/how much to pad tok with */
> +    prepend = g_strnfill((4 - strlen(tmptok)), '0');
> +
> +    /* combine it all together */
> +    if (prepend == NULL) {
> +        ret = g_strdup_printf("0.0.%s", tmptok);
> +    }
> +    else {
> +        ret = g_strdup_printf("0.0.%s%s", prepend, tmptok);
> +    }
> +    g_free (prepend);
> +    g_free (lcdev);
> +
> +    return ret;
> +}
> +
> +/**
> + * bd_s390_zfcp_sanitize_wwpn_input:
> + * @dev a zFCP WWPN identifier

Missing colon.
> + * @error: (out): place to store error (if any)
> + *
> + * Returns: (transfer full): a synthesized zFCP WWPN

Mention the return value needs to be freed.

> + */
> +gchar* bd_s390_zfcp_sanitize_wwpn_input (gchar *wwpn, GError **error) {
> +    gchar *fullwwpn = NULL;
> +    gchar *lcwwpn = NULL;
> +
> +    /* first make sure we're not being played */
> +    if ((wwpn == NULL) || (!*wwpn)) {
> +        g_set_error (error, BD_S390_ERROR, BD_S390_ERROR_DEVICE,
> +                     "You have not specified a WWPN or the WWPN is invalid");
> +        g_clear_error (error);
> +        return "";
> +    }
> +
> +    /* convert everything to lowercase */
> +    lcwwpn = g_ascii_strdown (wwpn, -1);
> +
> +    if (strncmp(lcwwpn, "0x", 2) == 0) {
> +        /* user entered a properly formatted wwpn */
> +        fullwwpn = g_strdup_printf("%s", lcwwpn);
> +    }
> +    else {
> +        fullwwpn = g_strdup_printf("0x%s", lcwwpn);
> +    }
> +    g_free (lcwwpn);
> +    return fullwwpn;
> +}
> +
> +/**
> + * bd_s390_zfcp_sanitize_lun_input:
> + * @dev a zFCP LUN identifier

Missing colon.

> + * @error: (out): place to store error (if any)
> + *
> + * Returns: (transfer full): a synthesized zFCP LUN

Mention the return value needs to be freed.

> + */
> +gchar* bd_s390_zfcp_sanitize_lun_input (gchar *lun, GError **error) {
> +    gchar *lclun = NULL;
> +    gchar *tmplun = NULL;
> +    gchar *fulllun = NULL;
> +    gchar *prepend = NULL;
> +    gchar *append = NULL;
> +
> +    /* first make sure we're not being played */
> +    if ((lun == NULL) || (!*lun)) {
> +        g_set_error (error, BD_S390_ERROR, BD_S390_ERROR_DEVICE,
> +                     "You have not specified a LUN or the LUN is invalid");
> +        g_clear_error (error);
> +        return "";
> +    }
> +
> +    /* convert everything to lowercase */
> +    lclun = g_ascii_strdown (lun, -1);
> +
> +    if ((g_str_has_prefix(lclun, "0x")) && (strlen(lclun) == 18)) {
> +        /* user entered a properly formatted lun */
> +        fulllun = g_strdup_printf ("%s", lclun);
> +    }
> +    else {
> +        /* we need to mangle the input to make it proper. ugh. */
> +        if (g_str_has_prefix (lclun, "0x")) {
> +            /* this may seem odd, but it makes the math easier a ways down */
> +            tmplun = lclun + 2;
> +        }
> +        else {
> +            tmplun = lclun;
> +        }
> +
> +        if (strlen(tmplun) < 4) {
> +            /* check if/how many zeros we pad to the left */
> +            prepend = g_strnfill((4 - strlen(tmplun)), '0');
> +            /* check if/how many zeros we pad to the right */
> +            append = g_strnfill((16 - (strlen(tmplun) + strlen(prepend))), '0');
> +        }
> +        else {
> +            /* didn't need to pad anything on the left; so just check if/how
> +             * many zeros we pad to the right */
> +            append = g_strnfill((16 - (strlen(tmplun))), '0');
> +        }
> +
> +        /* now combine everything together */
> +        if (prepend == NULL) {
> +            fulllun = g_strdup_printf ("0x%s%s", tmplun, append);
> +        }
> +        else {
> +            fulllun = g_strdup_printf ("0x%s%s%s", prepend, tmplun, append);
> +        }
> +    }
> +    g_free (lclun);
> +    g_free (prepend);
> +    g_free (append);
> +
> +    return fulllun;
> +}
> diff --git a/src/plugins/s390.h b/src/plugins/s390.h
> new file mode 100644
> index 0000000..5854fd8
> --- /dev/null
> +++ b/src/plugins/s390.h
> @@ -0,0 +1,24 @@
> +#include <glib.h>
> +#include <linux/fs.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <utils.h>
> +#include <asm/dasd.h>
> +#include <s390utils/vtoc.h>
> +
> +
> +GQuark bd_s390_error_quark (void);
> +#define BD_S390_ERROR bd_s390_error_quark ()
> +typedef enum {
> +    BD_S390_ERROR_DEVICE,
> +    BD_S390_ERROR_FORMAT_FAILED,
> +    BD_S390_ERROR_DASDFMT,
> +} BDS390Error;
> +
> +gboolean bd_s390_dasd_format (gchar *dasd, GError **error);
> +gboolean bd_s390_dasd_needs_format (gchar *dasd, GError **error);
> +gboolean bd_s390_dasd_online (gchar *dasd, GError **error);
> +gboolean bd_s390_dasd_is_ldl (gchar *dasd, GError **error);
> +gchar* bd_s390_sanitize_dev_input (gchar *dev, GError **error);
> +gchar* bd_s390_zfcp_sanitize_wwpn_input (gchar *wwpn, GError **error);
> +gchar* bd_s390_zfcp_sanitize_lun_input (gchar *lun, GError **error);
> diff --git a/src/python/gi/overrides/BlockDev.py b/src/python/gi/overrides/BlockDev.py
> index 5e47f03..2282643 100644
> --- a/src/python/gi/overrides/BlockDev.py
> +++ b/src/python/gi/overrides/BlockDev.py
> @@ -45,6 +45,7 @@ bd_plugins = { "lvm": BlockDev.Plugin.LVM,
>                  "swap": BlockDev.Plugin.SWAP,
>                  "mdraid": BlockDev.Plugin.MDRAID,
>                  "mpath": BlockDev.Plugin.MPATH,
> +               "s390": BlockDev.Plugin.S390,
>   }
>   
>   _init = BlockDev.init
> @@ -258,6 +259,41 @@ def swap_swapon(device, priority=-1):
>   __all__.append("swap_swapon")
>   
>   
> +_s390_dasd_format = BlockDev.s390_dasd_format
> +def s390_dasd_format(dasd):
> +    return _s390_dasd_format(dasd)
> +__all__.append("s390_dasd_format")
> +
> +_s390_dasd_needs_format = BlockDev.s390_dasd_needs_format
> +def s390_dasd_needs_format(dasd):
> +    return _s390_dasd_needs_format(dasd)
> +__all__.append("s390_dasd_needs_format")
> +
> +_s390_dasd_online = BlockDev.s390_dasd_online
> +def s390_dasd_online(dasd):
> +    return _s390_dasd_online(dasd)
> +__all__.append("s390_dasd_online")
> +
> +_s390_dasd_is_ldl = BlockDev.s390_dasd_is_ldl
> +def s390_dasd_is_ldl(dasd):
> +    return _s390_dasd_is_ldl(dasd)
> +__all__.append("s390_dasd_is_ldl")
> +
> +_s390_sanitize_dev_input = BlockDev.s390_sanitize_dev_input
> +def s390_sanitize_dev_input(dev):
> +    return _s390_sanitize_dev_input(dev)
> +__all__.append("s390_sanitize_dev_input")
> +
> +_s390_zfcp_sanitize_wwpn_input = BlockDev.s390_zfcp_sanitize_wwpn_input
> +def s390_zfcp_sanitize_wwpn_input(wwpn):
> +    return _s390_zfcp_sanitize_wwpn_input(wwpn)
> +__all__.append("s390_zfcp_sanitize_wwpn_input")
> +
> +_s390_zfcp_sanitize_lun_input = BlockDev.s390_zfcp_sanitize_lun_input
> +def s390_zfcp_sanitize_lun_input(lun):
> +    return _s390_zfcp_sanitize_lun_input(lun)
> +__all__.append("s390_zfcp_sanitize_lun_input")
> +
>   ## defined in this overrides only!
>   def plugin_specs_from_names(plugin_names):
>       ret = []
> @@ -424,6 +460,10 @@ class SwapError(BlockDevError):
>       pass
>   __all__.append("SwapError")
>   
> +class S390Error(BlockDevError):
> +    pass
> +__all__.append("S390Error")
> +
>   class UtilsError(BlockDevError):
>       pass
>   __all__.append("UtilsError")
> @@ -458,5 +498,8 @@ __all__.append("mpath")
>   swap = ErrorProxy("swap", BlockDev, [(GLib.Error, SwapError)], [not_implemented_rule])
>   __all__.append("swap")
>   
> +s390 = ErrorProxy("s390", BlockDev, [(GLib.Error, S390Error)], [not_implemented_rule])
> +__all__.append("s390")
> +
>   utils = ErrorProxy("utils", BlockDev, [(GLib.Error, UtilsError)])
>   __all__.append("utils")


I also noticed that a few of the doc comments in the .api file don't 
match the comments in the .c file, for example 
bd_s390_zfcp_sanitize_wwpn_input uses wwpn as a paremeter instead of 
dev. I have no idea if that's a big deal or not.


More information about the anaconda-patches mailing list