[Qemu-devel] [PATCH 0/3]: block: Small drive_init() improvements

[Qemu-devel] [PATCH 0/3]: block: Small drive_init() improvements

[Luiz Capitulino]

Please, see individual patches for details.

 blockdev.c |   65 ++++++++++++++++++++++++++++-------------------------------
 1 files changed, 31 insertions(+), 34 deletions(-)

[Qemu-devel] [PATCH 1/3] block: drive_init(): Fix indentation

[Luiz Capitulino]

Some constructions in that function have broken indentation. Fix them.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 blockdev.c |   52 ++++++++++++++++++++++++++--------------------------
 1 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 7d579d6..27bf68a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -272,23 +272,23 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
         if (type == IF_COUNT) {
             error_report("unsupported bus type '%s'", buf);
             return NULL;
-	}
+	    }
     }
     max_devs = if_max_devs[type];
 
     if (cyls || heads || secs) {
         if (cyls < 1 || (type == IF_IDE && cyls > 16383)) {
             error_report("invalid physical cyls number");
-	    return NULL;
-	}
+	        return NULL;
+	    }
         if (heads < 1 || (type == IF_IDE && heads > 16)) {
             error_report("invalid physical heads number");
-	    return NULL;
-	}
+	        return NULL;
+	    }
         if (secs < 1 || (type == IF_IDE && secs > 63)) {
             error_report("invalid physical secs number");
-	    return NULL;
-	}
+	        return NULL;
+	    }
     }
 
     if ((buf = qemu_opt_get(opts, "trans")) != NULL) {
@@ -297,31 +297,31 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
                          buf);
             return NULL;
         }
-        if (!strcmp(buf, "none"))
+        if (!strcmp(buf, "none")) {
             translation = BIOS_ATA_TRANSLATION_NONE;
-        else if (!strcmp(buf, "lba"))
+        } else if (!strcmp(buf, "lba")) {
             translation = BIOS_ATA_TRANSLATION_LBA;
-        else if (!strcmp(buf, "auto"))
+        } else if (!strcmp(buf, "auto")) {
             translation = BIOS_ATA_TRANSLATION_AUTO;
-	else {
+        } else {
             error_report("'%s' invalid translation type", buf);
-	    return NULL;
-	}
+	        return NULL;
+	    }
     }
 
     if ((buf = qemu_opt_get(opts, "media")) != NULL) {
         if (!strcmp(buf, "disk")) {
-	    media = MEDIA_DISK;
-	} else if (!strcmp(buf, "cdrom")) {
+	        media = MEDIA_DISK;
+	    } else if (!strcmp(buf, "cdrom")) {
             if (cyls || secs || heads) {
                 error_report("'%s' invalid physical CHS format", buf);
-	        return NULL;
+	            return NULL;
             }
-	    media = MEDIA_CDROM;
-	} else {
-	    error_report("'%s' invalid media", buf);
-	    return NULL;
-	}
+	        media = MEDIA_CDROM;
+	    } else {
+	        error_report("'%s' invalid media", buf);
+	        return NULL;
+	    }
     }
 
     if ((buf = qemu_opt_get(opts, "cache")) != NULL) {
@@ -480,17 +480,17 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
     case IF_XEN:
     case IF_NONE:
         switch(media) {
-	case MEDIA_DISK:
+	    case MEDIA_DISK:
             if (cyls != 0) {
                 bdrv_set_geometry_hint(dinfo->bdrv, cyls, heads, secs);
                 bdrv_set_translation_hint(dinfo->bdrv, translation);
             }
-	    break;
-	case MEDIA_CDROM:
+	        break;
+	    case MEDIA_CDROM:
             bdrv_set_removable(dinfo->bdrv, 1);
             dinfo->media_cd = 1;
-	    break;
-	}
+	        break;
+	    }
         break;
     case IF_SD:
         /* FIXME: This isn't really a floppy, but it's a reasonable
-- 
1.7.6.49.g033c2

Re: [Qemu-devel] [PATCH 1/3] block: drive_init(): Fix indentation

[Stefan Weil]

Am 30.06.2011 20:33, schrieb Luiz Capitulino:
> Some constructions in that function have broken indentation. Fix them.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
> blockdev.c | 52 ++++++++++++++++++++++++++--------------------------
> 1 files changed, 26 insertions(+), 26 deletions(-)
>

Hi,

the new indentation still uses tabs. Please check your patch
using scripts/checkpatch.pl.

Regards,
Stefan W.

Re: [Qemu-devel] [PATCH 1/3] block: drive_init(): Fix indentation

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> Some constructions in that function have broken indentation. Fix them.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  blockdev.c |   52 ++++++++++++++++++++++++++--------------------------
>  1 files changed, 26 insertions(+), 26 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 7d579d6..27bf68a 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -272,23 +272,23 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>          if (type == IF_COUNT) {
>              error_report("unsupported bus type '%s'", buf);
>              return NULL;
> -	}
> +	    }

Old indentation is correct, apart from tabs.

New indentation is incorrect, and fails to remove tabs.

        if (type == IF_COUNT) {
            error_report("unsupported bus type '%s'", buf);
            return NULL;
	    }
^^^^^^^^    ^
tab here    incorrect indentation

Check your editor settings.

I don't see "broken" indentation anywhere in this function.  A few lines
are off: indented 3 instead of 4.  If we want to fix such things, I'd
prefer it done globally.

Re: [Qemu-devel] [PATCH 1/3] block: drive_init(): Fix indentation

[Luiz Capitulino]

On Fri, 01 Jul 2011 09:16:38 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > Some constructions in that function have broken indentation. Fix them.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  blockdev.c |   52 ++++++++++++++++++++++++++--------------------------
> >  1 files changed, 26 insertions(+), 26 deletions(-)
> >
> > diff --git a/blockdev.c b/blockdev.c
> > index 7d579d6..27bf68a 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -272,23 +272,23 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
> >          if (type == IF_COUNT) {
> >              error_report("unsupported bus type '%s'", buf);
> >              return NULL;
> > -	}
> > +	    }
> 
> Old indentation is correct, apart from tabs.
> 
> New indentation is incorrect, and fails to remove tabs.
> 
>         if (type == IF_COUNT) {
>             error_report("unsupported bus type '%s'", buf);
>             return NULL;
> 	    }
> ^^^^^^^^    ^
> tab here    incorrect indentation

Yeah, I realized that after Stefan's comment.

> Check your editor settings.
> 
> I don't see "broken" indentation anywhere in this function.  A few lines
> are off: indented 3 instead of 4.  If we want to fix such things, I'd
> prefer it done globally.

Ok, I'll drop this patch from the series then and let to do it globally
later.

[Qemu-devel] [PATCH 2/3] block: drive_init(): Simplify interface type setting

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 blockdev.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 27bf68a..0a90ae8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -241,13 +241,6 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
 
     translation = BIOS_ATA_TRANSLATION_AUTO;
 
-    if (default_to_scsi) {
-        type = IF_SCSI;
-        pstrcpy(devname, sizeof(devname), "scsi");
-    } else {
-        type = IF_IDE;
-        pstrcpy(devname, sizeof(devname), "ide");
-    }
     media = MEDIA_DISK;
 
     /* extract parameters */
@@ -273,7 +266,11 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
             error_report("unsupported bus type '%s'", buf);
             return NULL;
 	    }
+    } else {
+        type = default_to_scsi ? IF_SCSI : IF_IDE;
+        pstrcpy(devname, sizeof(devname), if_name[type]);
     }
+
     max_devs = if_max_devs[type];
 
     if (cyls || heads || secs) {
-- 
1.7.6.49.g033c2

Re: [Qemu-devel] [PATCH 2/3] block: drive_init(): Simplify interface type setting

[Markus Armbruster]

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH 3/3] block: drive_init(): Improve CHS setting error message

[Luiz Capitulino]

The current media doesn't clearly say the error cause.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 blockdev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 0a90ae8..2dbdd1b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -311,7 +311,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
 	        media = MEDIA_DISK;
 	    } else if (!strcmp(buf, "cdrom")) {
             if (cyls || secs || heads) {
-                error_report("'%s' invalid physical CHS format", buf);
+                error_report("CHS can't be set for CDROM media '%s'", buf);
 	            return NULL;
             }
 	        media = MEDIA_CDROM;
-- 
1.7.6.49.g033c2

Re: [Qemu-devel] [PATCH 3/3] block: drive_init(): Improve CHS setting error message

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> The current media doesn't clearly say the error cause.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  blockdev.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 0a90ae8..2dbdd1b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -311,7 +311,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>  	        media = MEDIA_DISK;
>  	    } else if (!strcmp(buf, "cdrom")) {
>              if (cyls || secs || heads) {
> -                error_report("'%s' invalid physical CHS format", buf);
> +                error_report("CHS can't be set for CDROM media '%s'", buf);
>  	            return NULL;
>              }
>  	        media = MEDIA_CDROM;

It's an improvement.  I'd like "CHS can't be set with media=%s" even
better, because it's closer to the actual option string.  Matter of
taste.