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

Vratislav Podzimek vpodzime at redhat.com
Sat May 16 06:49:00 UTC 2015


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.

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

-- 
Vratislav Podzimek

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



More information about the anaconda-patches mailing list