[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