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

Samantha N. Bueno sbueno+anaconda at redhat.com
Fri May 15 20:10:38 UTC 2015


On Fri, May 15, 2015 at 09:55:04AM +0200, Vratislav Podzimek wrote:
> On Wed, 2015-05-13 at 14:16 -0400, Samantha N. Bueno wrote:
> > +
> > +/**
> > + *  * 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?

Yes.... copy-paste and then forgetting to remove them.
 
> > +
> > +/**
> > + * 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.

Yes, I was having some issue when I tried to use
bd_utils_exec_and_report_error, but I believe I know what my problem
might have been now. So I will make these changes.
 
> > +
> > +/**
> > + * 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.

Hmm, I'm surprised GCC didn't complain to me about this. Or maybe it was
something I changed at the last moment before posting. Either way,
fixed; and moved the g_free() to right after the 'fd =' assignment.
 
> > +
> > +    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.
 
It's hard to argue with that; fixed.

> > +
> > +    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. 

Today I learned that I can not count. Thanks!
 
> > +
> > +/**
> > + * 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.

Hmm, but if fd is NULL and I need to retry opening 'path' (a few lines
down), isn't it best that I don't free 'path' right here?
 
> > +
> > +    /* 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?

I'm *pretty* sure, but I can actually shorten this function a lot.
Instead of opening that "online" file, writing a "1" to it, closing the
file, etc., I can just use the "chccwdev" utility to switch a device's
online/offline status.
 
> > +
> > +/**
> > + * 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.

Ah, ok, fixed.
 
> > +    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. :)

Thank you!

Samantha


More information about the anaconda-patches mailing list