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

Vratislav Podzimek vpodzime at redhat.com
Tue May 19 18:11:24 UTC 2015


On Tue, 2015-05-19 at 12:22 -0400, Samantha N. Bueno wrote:
> On Tue, May 19, 2015 at 01:37:25PM +0200, Vratislav Podzimek wrote:
> > On Mon, 2015-05-18 at 15:10 -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                  | 414 ++++++++++++++++++++++++++++++++++++
> > >  src/plugins/s390.h                  |  79 +++++++
> > >  src/python/gi/overrides/BlockDev.py |  50 +++++
> > >  5 files changed, 653 insertions(+)
> > >  create mode 100644 src/lib/plugin_apis/s390.api
> > >  create mode 100644 src/plugins/s390.c
> > >  create mode 100644 src/plugins/s390.h
> > > 
> > > +
> > > +/**
> > > + * 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
> > Mismatch in the name of the parameter.
>  
> Fixed.
> 
> > > + * @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
> > Same here.
> 
> Fixed. Also fixed in the api file, and added the "Returns: (transfer
> full): ...." to these sanitizing functions.
>  
> > > +
> > > +/**
> > > + * 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);
> > Please move the long string above to the next line.
> 
> Done.
>  
> > > +        g_clear_error (error);
> > > +        return FALSE;
> > > +    }
> > > +
> > > +    rc = fgets (status, 12, fd);
> > You should do 
> >   status[11] = '\0';
> > here to make sure there is a null-byte at the very end (as you treat the
> > buffer as a string below). What are the other possible contents of the
> > file?
> 
> Done. And the "status" file will only be "unformatted" or "online", so
> if it's actually less than 12 chars (e.g. status is "online"), is it
> still a good/ok idea to set status[11] = '\0'; ?
Then it's probably best to call memset() on the buffer a zero it out
before you store anything into it. That way you'd for sure get a
null-byte after what's read.

> 
> Also, I'll add a comment stating the two possible values of "status".
>  
> > > +    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) {
> > Here you rely on 'status' being a string (null-terminated) since you try
> > to get it's length. Using 11 (as the length of the static string
> > "unformatted") should work the same way here.
> 
> Hmm, but if the status is "online" this might be a problem, to specify
> exactly 11 chars.
If the memset() is used to zero out the buffer first, you can use
strlen(status).

>  
> > > +        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;
> > > +        }
> > Should this really return if the 'dasd_cio_free' succeeds?
> 
> Gah, should be !rc, thanks.
>  
> > > +        /* 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);
> > Again a long string that should go to a separate line.
> 
> Fixed.
>  
> > > +            g_clear_error (error);
> > > +            return FALSE;
> > > +        }
> > > +    }
> > 'path' should be free'd in an else branch here.
> 
> 'path' is free'd unconditionally a few lines above though .... ?
> But yes, it should be free'd outside of this block, since this code
> path may not be followed (e.g. if a disk is not "online", but does not
> need to be to have dasd_cio_free run)
Exactly. And it shouldn't be free'd twice, thus the else branch.

> 
> > > +
> > > +    /* 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);
> > You probably don't want to set error here and clear it immediately.
> 
> Yeah, leftover crud from earlier attempts at stuff.
>  
> > > +        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);
> > Same here.
> 
> "     "
>  
> > > +        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);
> > > +        g_clear_error (error);
> > 'error' is/may not be set here, there's no reason to clear it.
> 
> Good point, removed.
>  
> > > +        return FALSE;
> > > +    }
> > > +
> > > +    return TRUE;
> > > +}
> > > +
> > > +/**
> > > + * bd_s390_dasd_read_vlabel:
> > > + * @dasd_info:
> > > + * @fd: a file descriptor for the open DASD device
> > > + * @blksize:
> > > + * @vlabel:
> > > + *
> > > + * Returns:
> > Wrong function name in the header and missing docs. Feel free to remove
> > them entirely as this is an internal function anyway.
> 
> K, removed.
>  
> > > + */
> > > +static int dasd_read_vlabel(dasd_information2_t *dasd_info, int fd, int blksize, volume_label_t *vlabel) {
> > > +    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;
> > > +    }
> > The fact that read() doesn't read the number of bytes requested doesn't
> > necessarily mean that there has been an error (it, for example, might
> > have been interrupted by a signal). You should check rc for being -1
> > (ERROR) and if it's less than sizeof(volume_label_t), you should try
> > reading more until you get -1 or 0 at which point either enough was read
> > (OK) or there's nothing more to read (ERROR).
> > Or you could switch to GLib's GIOChannel that, AFAICT, handles that
> > internally (just make sure to set encoding to NULL, default is utf-8)
> 
> If enough was read, I'm returned the number of bytes read, not 0. I'm
> not sure I understand "....reading more until you get -1 or 0...." since
> read() should only return bytes read or -1 in an error. I can add a
> check for if rc == -1 and, I dunno, try to read() again(?), but beyond
> that I don't think there's a lot else we can accomodate for.


>  
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +/**
> > > + * 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 ret = 0;
> > > +    gint blksize = 0;
> > > +    dasd_information2_t dasd_info;
> > > +    volume_label_t vlabel;
> > > +
> > > +    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;
> > > +    }
> > > +    if (ioctl(f, BLKSSZGET, &blksize) != 0) {
> > > +        close(f);
> > > +        return FALSE;
> > > +    }
> > > +
> > > +    if (ioctl(f, BIODASDINFO2, &dasd_info) != 0) {
> > > +        close(f);
> > > +        return FALSE;
> > > +    }
> > > +
> > > +    ret = dasd_read_vlabel(&dasd_info, f, blksize, &vlabel);
> > The 'vlabel' doesn't seem to be used anywhere below.
> 
> Right, but vlabel is a required parameter for calling dasd_read_vlabel.
But we don't have to call it here, do we? For what purpose?

>  
> > > +    close(f);
> > > +    if (ret == 2) {
> > > +        return FALSE;
> > > +    }
> > What about 'ret == 1'? Maybe check '!= 0' here instead?
> 
> Hmm, yes, I think this change should be ok.
>  
> > > +
> > > +    if (strncmp(dasd_info.type, "FBA", 3) == 0) {
> Thought this line might seem "odd" to someone so added a comment.
> Basically, you can not run dasdfmt on an FBA DASD, so that is why this
> check is performed.
> 
> > > +        return FALSE;
> > > +    }
> > > +
> > > +    /* check dasd volume label; "VOL1" is a CDL formatted DASD, won't require formatting */
> > Should use 'vlabel' somewhere, maybe?
> 
> It's a lot simpler to check dasd_info.format actually, since this
> directly contains the information needed, whereas nothing in the
> volume_label_t struct (of which vlabel is) is terribly useful for a
> simple check.
Then we don't need it at all and we should not read it for no reason.

-- 
Vratislav Podzimek

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



More information about the anaconda-patches mailing list