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

Vratislav Podzimek vpodzime at redhat.com
Sat May 16 07:13:36 UTC 2015


On Wed, 2015-05-13 at 14:16 -0400, Samantha N. Bueno wrote:
> +/**
> + * bd_s390_dasd_is_ldl:
> + * @dasd: dasd to check, whether it is LDL formatted
Missing doc for the @error parameter.

> + *
> + * Returns: whether a dasd is LDL formatted
> + */
> +gboolean bd_s390_dasd_is_ldl (gchar *dasd, GError **error) {
> +    gchar devname[16];
> +    gint f = 0;
> +    gint ret = 0;
> +    gint blksize = 0;
> +    dasd_information2_t dasd_info;
> +    volume_label_t vlabel;
> +
> +    memset(&dasd_info, 0, sizeof(dasd_info));
> +    strcpy(devname, "/dev/");
> +    strcat(devname, dasd);
Are you sure 16 characters will always be enough? If you are, you should
use strncat() and add a check-and-report-error block verifying that. If
not, you should switch to g_strdup_printf() and a heap-allocated string
because "the dest string must have enough space for the result.  If dest
is not large enough, program behavior is unpredictable; buffer overruns
are a favorite avenue for attacking secure programs." (from 'man
strcat')

> +
> +    if ((f = open(devname, O_RDONLY)) == -1)
> +        g_set_error (error, BD_S390_ERROR, BD_S390_ERROR_DEVICE, "Unable to open device %s", devname);
> +        return FALSE;
Missing {}s here, returns FALSE always. If you were using Emacs, I'd
tell you to select the whole file and hit TAB (aka "indent to were it
belongs") to reveal these gotchas, but I have no idea how to do it in
Vim.

> +    if (ioctl(f, BLKSSZGET, &blksize) != 0) {
> +        close(f);
> +        return FALSE;
> +    }
> +
> +    if (ioctl(f, BIODASDINFO2, &dasd_info) != 0) {
> +        close(f);
> +        return FALSE;
> +    }
> +
> +    ret = bd_s390_dasd_read_vlabel(&dasd_info, f, blksize, &vlabel);
> +    close(f);
> +    if (ret == 2)
> +        return FALSE;
> +
> +    if (strncmp(dasd_info.type, "FBA", 3) == 0) {
> +        return FALSE;
> +    }
> +
> +    if (dasd_info.format == DASD_FORMAT_CDL) { /* VOL1, CDL */
I don't understand the comment above.

> +        return FALSE;
> +    }
> +    else {
> +        return TRUE;
> +    }
> +}
> +
> +/**
> + * 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
Would be better to specify ownership of the result explicitly:

Returns: (transfer full): a synthesized...

> + */
> +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, strlen(dev));
> +
> +    /* 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 */
tmptok = tok + 1;

would look much better to me.

> +    }
> +    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);
> +    }
> +    return ret;
> +}
> +
> +/**
> + * bd_s390_zfcp_sanitize_wwpn_input:
> + * @dev a zFCP WWPN identifier
> + * @error: (out): place to store error (if any)
> + *
> + * Returns: a synthesized zFCP WWPN
Same comment about the result ownership here.

> + */
> +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, strlen(wwpn));
> +
> +    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);
> +    }
> +    return fullwwpn;
> +}
> +
> +/**
> + * 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) {
> +    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, strlen(lun));
g_ascii_strdown() returns a newly-allocated string that should be free'd
somewhere.

> +
> +    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];
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);
> +            g_free (append);
> +        }
> +        else {
> +            fulllun = g_strdup_printf ("0x%s%s%s", prepend, tmplun, append);
> +            g_free (prepend);
> +            g_free (append);
> +        }
> +    }
I think it would be easier to just free prepend, append and lclun here
unconditionally ('g_free (NULL)' is okay).

> +    return fulllun;
> +}
> diff --git a/src/plugins/s390.h b/src/plugins/s390.h
> new file mode 100644
> index 0000000..52cb3ab
> --- /dev/null
> +++ b/src/plugins/s390.h
> @@ -0,0 +1,79 @@
> +#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;
> +
> +/**
> + * 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_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_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_cdl:
> + * @dasd: dasd to check, whether it is CDL formatted
> + * @error: (out): place to store error (if any)
> + *
> + * Returns: whether a dasd is CDL formatted
> + */
> +gboolean bd_s390_dasd_is_ldl (gchar *dasd, 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/python/gi/overrides/BlockDev.py b/src/python/gi/overrides/BlockDev.py
> index 5e47f03..404e5a8 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,48 @@ def swap_swapon(device, priority=-1):
>  __all__.append("swap_swapon")
>  
> 
> +_s390_dasd_format = BlockDev.s390_dasd_format
> + at override(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
> + at override(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
> + at override(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
> + at override(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
> + at override(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
> + at override(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
> + at override(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 +467,10 @@ class SwapError(BlockDevError):
>      pass
>  __all__.append("SwapError")
>  
> +class S390Error(BlockDevError):
> +    pass
> +__all__.append("S390Error")
> +
>  class UtilsError(BlockDevError):
>      pass
>  __all__.append("UtilsError")
> @@ -458,5 +505,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")
Otherwise looks good to me. Thanks for your work on this!

-- 
Vratislav Podzimek

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



More information about the anaconda-patches mailing list