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

Samantha N. Bueno sbueno+anaconda at redhat.com
Mon May 18 00:29:02 UTC 2015


On Sat, May 16, 2015 at 08:49:00AM +0200, Vratislav Podzimek wrote:
> On Fri, 2015-05-15 at 16:10 -0400, Samantha N. Bueno wrote:
> > > > +
> > > > +/**
> > > > + * 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?
> Good point, I missed the usage below. But then you need to free 'path'
> before every return until it is free'd unconditionally.

Yeah, I know. :-/
 
> >  
> > > > +
> > > > +    /* 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.
> If we can avoid running utilities, we should do so. But of course we
> could start with the easier implementation of the function and then
> replace it with a more optimal one.

Ok, that's fair. All I had to do to make this work was call rewind() to
reset the file cursor, so I've avoided running another utility then. :)
 
Samantha


More information about the anaconda-patches mailing list