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

Samantha N. Bueno sbueno+anaconda at redhat.com
Wed May 20 02:30:31 UTC 2015


On Tue, May 19, 2015 at 08:11:24PM +0200, Vratislav Podzimek wrote:
> 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
> > > > 
> > > > +        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.

Aha, sounds great. So then if I memset() I should not need to have this
    status[11] = '\0';
line, from what I understand.
 
> > 
> > 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).

Perfect. memset() it is.
 
> > > > +            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.

Ok then. 
 
> > > > + */
> > > > +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.
> 
> 
>  
> > > > +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.

I guess not, so then the function read_vlabel is not needed either. I
copied it over from rhel6-branch and was wary to remove it, but after
testing it without reading vlabel, and after examining this function, I
believe it is safe to remove it entirely.

Will post the third and hopefully final iteration.


More information about the anaconda-patches mailing list